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/gstreamer not pushing tags 2 #1487

Closed
wants to merge 5 commits into
base: develop
from

Conversation

2 participants
@SeeSpotRun
Contributor

SeeSpotRun commented Mar 15, 2016

Take 2 (replaces #1485):

This applies a workaround for #935, #1453, #1474 and #1480 until gstreamer upstream fix percolates down (gstreamer 1.6.4 or 1.7.91).

I tested on my Arch box with previously problematic .flac files and gstreamer 1.6.3. All tags were picked up and there was no apparent loss of speed (relative to gstreamer compiled from git master).

The main fix is in 2eb43f2, the other two commits are just economising a bit of code and so can be left off if desired.

@SeeSpotRun SeeSpotRun force-pushed the SeeSpotRun:fix/gstreamer_not_pushing_tags_2 branch from 313c942 to 1101823 Mar 15, 2016

@SeeSpotRun

This comment has been minimized.

Contributor

SeeSpotRun commented Mar 15, 2016

Some timings comparisons:

gstreamer version mopidy branch scan time (s) tracks correctly tagged
1.4.5 develop 91 4117
1.4.5 this one 90 4117
1.6.3 develop 69 106
1.6.3 this one 126 4117
git pre-1.7.91 develop 88 4117
git pre-1.7.91 this one 88 4117

@SeeSpotRun SeeSpotRun force-pushed the SeeSpotRun:fix/gstreamer_not_pushing_tags_2 branch from a5d8dd7 to c10c393 Mar 15, 2016

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 16, 2016

Thank you so much for figuring this out, getting it all fixed upstream and taking the time to make this workaround and do the regression testing. Sadly I haven't had a chance to look more closely over the code, as things have been rather hectic lately. But we'll hopefully get it into a 2.0.1 release as soon as we can.

@SeeSpotRun

This comment has been minimized.

Contributor

SeeSpotRun commented Mar 19, 2016

No problems; I might see if I can get any of the correspondents at #935, #1474 and #1480 to test this fix.

@adamcik adamcik added this to the v2.0.1 - Bug fixes milestone Mar 25, 2016

@adamcik adamcik self-assigned this Mar 25, 2016

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 25, 2016

Cherry-picked over to release-2.0 and simplified the logic a bit in e9137e1 also added the missing changelog. Will be merged into develop shortly. Thanks for the fix :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment