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

[fix] MusicDatabase Songs set real path to avoid passing MusicDB URLs to player/demuxer #13357

Merged
merged 1 commit into from Jan 14, 2018

Conversation

@Voyager1
Copy link
Member

commented Jan 13, 2018

Description

Some music (flacs) could not be played back when started from the music library nodes, but did work properly when starting from the "files" or "all songs" node.

Motivation and Context

When playing music from music library nodes, the paths have a different format, which does NOT include the file extension at the very end of the string. Because of this, the demuxer doesn't know the content type and it tries to probe the file to determine the right codec. Some flacs (probably broken, examples provided by @DaveTBlake) are then incorrectly 'recognized' as mp3 therefore playback fails immediately after start.
When playing these same examples from the "files" node, they work fine, as there the file extension is at the end and the right codec is correctly selected.

The root cause is indeed that the file extension cannot be determined by the demuxer.
To solve this, all that needed to be done was to set the REAL path in m_strDynPath, when reading the item from the music database.

How Has This Been Tested?

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 requested a review from FernetMenta Jan 13, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

This is not the correct fix. The root cause is that file extensions are not passed to demuxer when played from music db. file extensions may be required. passing a music db url to ffmpeg is nonsense.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

music db should store the real fileneme m_strDynPath

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

Thanks @Voyager1 for looking at this.
I did look at this myself @FernetMenta but really could not see how CMusicDatabaseFile was ever meant to work, assuming it is this class that should be converting VFS path to something the player understands.

Also need to cover JSON API calls that add music library items to the queue, not just play form library GUI

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

@DaveTBlake you should remember that I looked at this a couple of weeks ago and told you the root cause. we are not going to implement a work-around that does only work for the case if content type was set by chance. VFS as is has hit its limits here. I see no advantage in passing around made-up URL like musicdb://.
Store the real filename in m_strDynPath when the fileitem is contructed.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2018

I think we agree on the root cause. I got so far with my analysis. My solution was a bit of a workaround, I agree, but I didn't know we don't want those music db URLs to be passed to the demuxer. All good, I'll take a look and update the PR. Shouldn't be too difficult it seems.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

btw: there is another flaw with VFS and MusicdatabaseFile. The original options are not passed to the embedded CFile. Thanks to crappy default parameters in Open
bool Open(const std::string& strFileName, const unsigned int flags = 0);
this issue got hidden

As a result the actual file might be played without caching when it should or vice versa

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

@FernetMenta sorry that I was unable to understand how to fix this even with your previous hints. It seems that the fundamental difficulty we have is that the player interface invoves CFileItem, and that could contain almost anything.

I hope that Voyager1 fares better. I am interested in what the solution is, and will happy test.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

Some VSF paths also do have file extension at the end e.g. musicdb://albums/1406/75599.flac and the player would be fine.
But they also have options at the end depending on the GUI navigation
e.g. musicdb://albums/1406/75599.flac?albumid=1406
I guess this could be contributing too. It explains why the test files do play from an unfiltered songs node.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

VFS has flaws and the design is exhausted. From a player pov is it not required. Set the real filename, CFileItem::SetDynURL, when the music db fileItems are contructed. I think this is here:
https://github.com/xbmc/xbmc/blob/master/xbmc/filesystem/MusicDatabaseDirectory.cpp#L40

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

I am a little confused. The real filename (and path) is already in CFileItem.GetMusicInfoTag()->GetURL(), and is used by PAPlayer::QueueNextFileEx, could not the demuxer do the same?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

That is only used to determine if we are in a CUE sheet and was done before we had DynURL. Can be migrated to DynURL too. Neither VideoPlayer nor PAPlayer are interested in fileItems having any InfoTags. They are just interested in the real file name and options passed along with fileItem.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

I agree file name and some options would make a much better interface than the current mess that is fileitem.

You mentioned cue sheets. Is embedded cuesheets (in FLAC files) the one situation where the PAPlayer does need to be interested in the MusicInfoTag, because that is where the cuesheet contents are?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

This MusicInfoTag hack for cue sheets was done (by myself) for the same reason we are discussing issues here: #3304
Things went wrong when items were played from db. PAPlayer did not know that the real file did not change because it was not give the real file name.
This time I want to get it right by fixing the root cause :)

@Voyager1 Voyager1 force-pushed the Voyager1:fix_flac_playfromlib branch from b384dee to cc11745 Jan 13, 2018
@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2018

@FernetMenta @DaveTBlake new update pushed, please check it out. Setting m_strDynPath upon reading the item from the database does the trick, and is sufficient because the downstream code nicely uses this value if it's available. It only needs to be done for actual Songs (not Artists or anything else). Any CMusicDatabase method that retrieves Songs eventually passes through GetFileItemFromDataset, where this is now fixed.

@Voyager1 Voyager1 changed the title [fix] some flac cannot be probed and require the use of content type [fix] MusicDatabase Songs set real path to avoid passing MusicDB URLs to player/demuxer Jan 13, 2018
Copy link
Member

left a comment

great

@Voyager1 Voyager1 merged commit 94bcd5c into xbmc:master Jan 14, 2018
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@DaveTBlake DaveTBlake added this to the L 18.0-alpha1 milestone Jan 14, 2018
@Voyager1 Voyager1 deleted the Voyager1:fix_flac_playfromlib branch Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.