After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 636686 - playlist with many files wrong order
playlist with many files wrong order
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: Movie player
2.32.x
Other Linux
: Normal normal
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2010-12-07 10:41 UTC by gnome.com
Modified: 2011-01-28 12:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add totem_playlist_add_mrls() API (12.62 KB, patch)
2010-12-15 16:37 UTC, Philip Withnall
committed Details | Review
Use the new totem_playlist_add_mrls() API (11.08 KB, patch)
2010-12-15 16:38 UTC, Philip Withnall
committed Details | Review
Second patch fuzzed for 2.32 (11.02 KB, patch)
2011-01-27 23:19 UTC, Edward Z. Yang
needs-work Details | Review

Description gnome.com 2010-12-07 10:41:33 UTC
Totem distributed with ubuntu maverick:
totem 2.32.0

Try this:

mkdir foo
cd foo
for a in  $(seq 10000 17000); do ln ../tmp.avi $a.avi; done
totem *

I've saved the playlist it looks like this:

[playlist]
NumberOfEntries=7001
File1=10000.avi
File2=10001.avi
File3=10003.avi
File4=10004.avi
File5=10005.avi
File6=10006.avi
File7=10002.avi
File8=10007.avi
File9=10008.avi
File10=10009.avi
File11=10010.avi
File12=10011.avi
File13=10013.avi
File14=10014.avi
File15=10012.avi
File16=10015.avi
File17=10016.avi
File18=10019.avi
File19=10017.avi
File20=10020.avi
...

Not quite the order expected...

Doing the same with gnome-mplayer yields a more expected result.
Comment 1 Bastien Nocera 2010-12-08 13:34:04 UTC
Does the playlist appear in the right order in the playlist sidebar? Or is it the parsing that has problems?
Comment 2 gnome.com 2010-12-08 18:00:20 UTC
Playlist sidebar is same order as saved playlist. (I only saved the list, so that I didn't have to type in all the filenames from the list).

I've also tried to compile the 2.32.0 from source (without ubuntu patches)
(apt-get build-dep totem; apt-get source totem; rm -rf totem-2.32.0;tar zxf totem_2.32.0.orig.tar.gz) and this exhibit same behavior.   

(Might be related to this: https://bugzilla.gnome.org/show_bug.cgi?id=634751 But I'm in no position to determine that)
Comment 3 Bastien Nocera 2010-12-08 18:16:36 UTC
It's not related to that other bug.

Which version of totem-pl-parser are you using? Make sure you test with totem-pl-parser 2.32.1 at the very least. Earlier versions have bugs related to the ordering of playlist parsing.
Comment 4 gnome.com 2010-12-08 22:43:19 UTC
Ubuntu comes with totem-pl-parser-2.30.3, I've built a 2.32.1:

unpacked 2.32.1
copied the "debian" subtree from 2.30.3-1ubuntu0,
added an entry to changelog
ran debian/rules binary
removed dependencies on missing gir-* files (for dev and gir packages)

installed:
libtotem-plparser17_2.32.1-0ubuntu1_i386.deb
libtotem-plparser-dev_2.32.1-0ubuntu1_i386.deb

this made no difference

recompiling totem against the new -dev package (not sure it's necessary, probably no abi change). This made no difference either.

The scary part is:
I ran "totem *" and saved the playlist twice, and the playlist differs.

To me that looks like a race-condition or an unfortunate pointer-compare at some point.
Comment 5 Philip Withnall 2010-12-15 16:37:05 UTC
Created attachment 176478 [details] [review]
Add totem_playlist_add_mrls() API

Patch 1 of 2. This adds a new totem_playlist_add_mrls() API, which guarantees to add playlist entries to the playlist asynchronously in the order they're listed in the parameters to the function.

This doesn't require any changes to totem-pl-parser, as there's no need to implement the changes at that level — doing so wouldn't enable us to introduce any more parallelism.
Comment 6 Philip Withnall 2010-12-15 16:38:11 UTC
Created attachment 176479 [details] [review]
Use the new totem_playlist_add_mrls() API

Patch 2 of 2. This converts Totem to use totem_playlist_add_mrls() everywhere.

This fixes the test case given in comment #1. Thanks for providing such a concise and useful set of steps for reproducing the problem!
Comment 7 Philip Withnall 2010-12-15 17:02:53 UTC
OKed by Bastien on IRC. Merged to master.

commit 8a810680960f0e7d2d9e10b6f21da21332a5bbfe
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Wed Dec 15 16:31:04 2010 +0000

    Use the new totem_playlist_add_mrls() API to add multiple playlist entries
    
    As described in the previous commit, this new API ensures that when adding
    multiple playlist entries asynchronously, they're not added to Totem's
    playlist out of order. Closes: bgo#636686

 src/plugins/publish/totem-publish.c |    7 +++-
 src/totem-object.c                  |   70 ++++++++++++++--------------------
 src/totem-playlist.c                |   28 ++++++++------
 src/totem-video-list.c              |    7 +++-
 4 files changed, 57 insertions(+), 55 deletions(-)

commit 68702722fecd6baf8e006ab5804cbcd760182c87
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Wed Dec 15 14:37:59 2010 +0000

    Add totem_playlist_add_mrls() API
    
    Due to various places in Totem calling the new asynchronous playlist parsing
    API on lists of items, it's now possible for playlist to be added to Totem in
    orders other than their input order. This is bad.
    
    This commit adds a new totem_playlist_add_mrls() API, which asynchronously
    adds lists of MRLs to Totem's playlist, preserving their input order.
    
    Helps: bgo#636686

 src/totem-playlist.c |  251 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/totem-playlist.h |   16 +++
 2 files changed, 267 insertions(+), 0 deletions(-)
Comment 8 Edward Z. Yang 2011-01-27 23:02:15 UTC
By any chance can we get this patch ported back to 2.32? It's effecting those users who are still on GTK2. I'd be more than happy to put in the work to make the patch apply cleanly.

https://bugs.launchpad.net/ubuntu/+source/totem/+bug/659001
Comment 9 Edward Z. Yang 2011-01-27 23:19:56 UTC
Created attachment 179476 [details] [review]
Second patch fuzzed for 2.32

First patch applied cleanly, second patch had some fuzz, but the end result is the same.
Comment 10 Bastien Nocera 2011-01-28 12:23:06 UTC
(In reply to comment #9)
> Created an attachment (id=179476) [details] [review]
> Second patch fuzzed for 2.32
> 
> First patch applied cleanly, second patch had some fuzz, but the end result is
> the same.

The patch breaks the publish plugin.
Comment 11 Bastien Nocera 2011-01-28 12:36:42 UTC
Looks like another patch was actually missing. Committed to gnome-2-32. Will be in the next stable release (if we end up doing one).

commit 67c4c2e07b00487391ab8b906360d3f3f09e9b84
Author: Bastien Nocera <hadess@hadess.net>
Date:   Fri Jan 28 12:35:31 2011 +0000

    publish: Make it compile again
    
    After the last cherry-pick from master.

commit a9ed336231663812c58ac0fb544d9b9a9862a90e
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Wed Dec 15 14:37:59 2010 +0000

    Add totem_playlist_add_mrls() API
    
    Due to various places in Totem calling the new asynchronous playlist parsing
    API on lists of items, it's now possible for playlist to be added to Totem in
    orders other than their input order. This is bad.
    
    This commit adds a new totem_playlist_add_mrls() API, which asynchronously
    adds lists of MRLs to Totem's playlist, preserving their input order.
    
    Helps: bgo#636686

commit 75f0ef09df10d3a1d6a8a3063c3258b128bc2e08
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Wed Dec 15 16:31:04 2010 +0000

    Use the new totem_playlist_add_mrls() API to add multiple playlist entries
    
    As described in the previous commit, this new API ensures that when adding
    multiple playlist entries asynchronously, they're not added to Totem's
    playlist out of order. Closes: bgo#636686