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

[bugfix] Stack Regression: synchronize m_itemCurrentFile with player state #13298

Closed
wants to merge 2 commits into from

Conversation

Voyager1
Copy link

@Voyager1 Voyager1 commented Jan 3, 2018

Description

As reported on the team slack channel, when playing music hitting "next" lost current item information.
Upon deeper investigation, the culprit was with how m_itemCurrentFile is not properly synchronized with player state. This PR addresses that issue.
Additionally two special issues had to be dealt with:

  • player doesn't always start gapless, in which case two player threads can be active (the one stopping plus the new one starting). In order to properly set m_itemCurrentFile, one must wait that the first player stopped before starting the new one.
  • because m_itemCurrentFile is now reset to null once player is finished, there are a few additional checks on the validity of m_itemCurrentFile before dereferencing...

Motivation and Context

see above

How Has This Been Tested?

extensive mix of music, video, dvd, stacks, iso stacks.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@Voyager1 Voyager1 added Type: Fix non-breaking change which fixes an issue v18 Leia labels Jan 3, 2018
m_appPlayer.ClosePlayer();
if (m_itemCurrentFile != nullptr)
{
// wait for m_itemCurrentFile being reset to nullptr (asynch job)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

m_appPlayer.ClosePlayerGapless(newPlayer);
if (m_appPlayer.HasPlayer())
{
if (m_appPlayer.CanClosePlayerGapless(newPlayer, item))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.


bool gaplessSupported = player->m_type == "music" || (player->m_type == "video" && !item.IsDiscImage() && !item.IsDVDFile());
gaplessSupported = gaplessSupported && (playername == player->m_name);
return gaplessSupported;

This comment was marked as spam.

This comment was marked as spam.

// but if we do not stop it, we can not distinguish callbacks from previous
// item and current item, it will confused us then we can not make correct delay
// callback after the starting state.
CloseFile(true);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@Voyager1
Copy link
Author

Voyager1 commented Jan 4, 2018

closing in favor of #13301

@Voyager1 Voyager1 closed this Jan 4, 2018
@Rechi Rechi removed the v18 Leia label Jan 4, 2018
@Voyager1 Voyager1 deleted the fix-stack-regression branch January 14, 2018 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants