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

Jump to bottom

fix and simplify PlayFile #13301

Merged
merged 2 commits into from Jan 5, 2018
Merged

fix and simplify PlayFile #13301

merged 2 commits into from Jan 5, 2018

Conversation

FernetMenta
Copy link
Contributor

Player start/stop is asynchronous now.

  • sync m_itemCurrentFile
  • drop obesolete return codes of PlayFile
  • simplify CApplication by moving some logic into CApplicationPlayer

@FernetMenta
Copy link
Contributor Author

@Voyager1 please have a look

zag2me
zag2me reviewed Jan 4, 2018
View reviewed changes
xbmc/ApplicationPlayer.cpp Outdated
else
newPlayer = CPlayerCoreFactory::GetInstance().GetDefaultPlayer(item);

// check fi we need to close current player

This comment was marked as spam.

Sign in to view

@Voyager1
Copy link

Voyager1 commented Jan 4, 2018

m_itemCurrentFile is never reset to null, is that on purpose?

@Voyager1
Copy link

Voyager1 commented Jan 4, 2018

something else to look into: code is also systematically crashing inside CDatabase::open when trying to start another playback while something is playing already.

@FernetMenta
Copy link
Contributor Author

@Voyager1 I can't repo the crash. what exactly are you doing to make it crash?

@FernetMenta
Copy link
Contributor Author

I think I found the issue. see last commit

@Voyager1
Copy link

Voyager1 commented Jan 4, 2018 •
edited

@FernetMenta thanks - I have tested the latest and it works.
To things:

  1. There is one quirk that needs fixing now, related to playback of iso stack not properly working anymore following your changes, due to premature clear of the helper. I fixed it here: Voyager1@173a15f

  2. Also there's this earlier fix that I believe you didn't carry over yet: Voyager1@8128add
    It's required because of the original code was setting/resetting m_itemCurrentFile startoffset to STARTOFFSET_RESUME in order to determine the fullscreen state (see original lines 3104-3105)..

I suggest you pull these 2 commits in. Other than that, looks great!! thanks a lot.

@Voyager1 Voyager1 mentioned this pull request Jan 4, 2018
Closed
10 tasks
@FernetMenta
Copy link
Contributor Author

FernetMenta commented Jan 4, 2018 •
edited

@Voyager1 do you want me to cherry-pick those 2 commits? what about the commit mesg "fixup" of 2nd?

@Voyager1
Copy link

Voyager1 commented Jan 4, 2018

yes please. The "fixup" message is because that's something that was missed as part of your first commit. it really belongs in there. Same for the other commit. You can squash them if you like.

@FernetMenta
Copy link
Contributor Author

jenkins build this please

@Voyager1
Copy link

Voyager1 commented Jan 5, 2018

jenkins seems ok (usual Test suite failure on OSX/Win64)

@DaveTBlake
Copy link
Member

This fixes the regression where hitting "next" when playing music causes current item information? Sorry but I can't make a local build at the moment to test myself.
Also would like to test using JSON API calls as that often seems to find yet another route through player from GUI

@Voyager1
Copy link

Voyager1 commented Jan 5, 2018 •
edited

This fixes the regression where hitting "next" when playing music causes current item information?

Yes it does.

@FernetMenta FernetMenta merged commit 7366d28 into xbmc:master Jan 5, 2018
@FernetMenta FernetMenta deleted the playfile branch January 5, 2018 16:34
@Rechi Rechi added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v18 Leia labels Jan 5, 2018
@Rechi Rechi added this to the L 18.0-alpha1 milestone Jan 5, 2018
popcornmix added a commit to popcornmix/xbmc that referenced this pull request Jan 12, 2018
@popcornmix
Application: Set volume after opening file
0664565 
Initial volume was broken in omxplayer after xbmc#13301
popcornmix added a commit to popcornmix/xbmc that referenced this pull request Jan 15, 2018
@popcornmix
Application: Set volume after opening file
8f9bf7b 
Initial volume was broken in omxplayer after xbmc#13301
@popcornmix popcornmix mentioned this pull request Jan 15, 2018
Merged
10 tasks
@DaveTBlake DaveTBlake mentioned this pull request Oct 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zag2me zag2me zag2me left review comments

Assignees
No one assigned
Labels
Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@FernetMenta @Voyager1 @DaveTBlake @zag2me @Rechi