Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added navigation controls for EDL (commercials) in PVR playback #10064

Merged
merged 1 commit into from Jul 6, 2016

Conversation

@thardie
Copy link
Contributor

commented Jul 3, 2016

Added functionality so that when playing from a PVR if there is an EDL, NEXT will go to the end of the next commercial block, and PREV will go to the beginning of the previous commercial block. If no EDL is present, old behavior is preserved.

@akva2

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2016

No real review but you will have to deal with it anyway. Indent is 2 spaces and braces go on separate lines.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 3, 2016

why did you limit it for pvr only?

@thardie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2016

@akva2 The second commit fixes the indenting and braces - Did I miss something?
@FernetMenta I haven't seen EDLs for any other kind of videos, so can't test it, and didn't want to break old behavior. Is it safe to remove the PVR restriction?

@@ -4355,6 +4355,21 @@ bool CVideoPlayer::OnAction(const CAction &action)
case ACTION_NEXT_ITEM:
case ACTION_CHANNEL_UP:
{
if (m_pInputStream->IsStreamType(DVDSTREAM_TYPE_PVRMANAGER))

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 3, 2016

Member

In v18 there won't be a streamtype PVR anymore. I would just drop this limitation.

@thardie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2016

@FernetMenta Removed the restriction as requested.

@@ -840,6 +840,50 @@ bool CEdl::InCut(const int iSeek, Cut *pCut) const
return false;
}

bool CEdl::GetNearestCut(bool bPlus, const int iSeek, Cut *pCut) const
{
if (bPlus) {

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 4, 2016

Member

curly braces next line please

{
if (bPlus) {
// Searching forwards
for (int i = 0; i < (int)m_vecCuts.size(); i++)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 4, 2016

Member

we moved to c++ 11 style loops: for (auto &cut : m_vecCuts)

This comment has been minimized.

Copy link
@thardie

thardie Jul 4, 2016

Author Contributor

There's another loop below this where I go through the list backwards - Do you want me to re-implement that in C++ 11 style as well?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 4, 2016

Member

hmm, there is no revers range based loop, right? then might be better keeping it as is.

This comment has been minimized.

Copy link
@Montellese

Montellese Jul 4, 2016

Member

But you could use iterators instead of indices. They exist for both directions (begin() and rbegin() etc.)

@@ -4355,6 +4355,18 @@ bool CVideoPlayer::OnAction(const CAction &action)
case ACTION_NEXT_ITEM:
case ACTION_CHANNEL_UP:
{
if (m_Edl.HasCut())
{
// For PVR streams, we'll do EDL searching, since they can't have next/prev

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jul 4, 2016

Member

I would drop reference to PVR in this comment

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 4, 2016

thanks, apart from the minors (see comments) I am ok with this.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 4, 2016

@thardie could you please squash the 4 commits into a single one. then we are good to go

@thardie thardie force-pushed the thardie:master branch from 95fb3e3 to 7754df9 Jul 4, 2016

@thardie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2016

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 5, 2016

there is a "merge remote tracking" commit in this branch. always rebase on upstream, never merge upstream

@thardie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2016

Oops, sorry. Will fix it this evening (approx 8 hours from now)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 5, 2016

thanks!

@thardie thardie force-pushed the thardie:master branch 2 times, most recently from 53e0eb8 to 8577295 Jul 6, 2016

terry terry
Added controls for Next item/Prev item in video playback with EDL
(commercials) to go to the end of the next commercial block, or
prev to the beginning of the previous commercial block.

@thardie thardie force-pushed the thardie:master branch from 8577295 to 35f3a77 Jul 6, 2016

@thardie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

@FernetMenta Should be good now.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

jenkins build this please

@FernetMenta FernetMenta merged commit 9311fff into xbmc:master Jul 6, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.