GNOME Bugzilla – Bug 639941
basesink: a position query done after EOS with negative playback rate should return 0
Last modified: 2018-05-04 09:43:21 UTC
Created attachment 178711 [details] call it with a URI in argument It seems that commit a813aad0acfe016625be885e20f88c9c7f741cf9 of -core introduces a regression in that case of position queries during reverse playback after EOS. With current pre-release the position query returns a non-null position. Attaching a simple test-case
Created attachment 178712 [details] simpler test case
Created attachment 178717 [details] [review] basesink: return null position on EOS during reverse playback. Reset the last seen timestamp if the sink is in EOS and segment playback rate is negative. This fixes a regression introduced by commit a813aad0acfe016625be885e20f88c9c7f741cf9.
As I understand it this was postponed for until after the release, with the test case not being entirely reliable either.
This is still a regression anyway. Because of that I can see 2 WebKit media tests consistently failing here.
Actually, I just re-read the logs and I think the consensus was that the patch should go in (but I also seem to have gathered that there are other/better solutions).
IRC log from yesterday: 10:28:17 <wtay> : __tim, do you think that small basesink patch could go in as well? 10:28:40 <tim> : wtay_, I was going to ask you about that :) 10:28:51 <wtay> : I think it should 10:28:55 <tim> : wtay_, it looks harmless 10:29:02 <wtay> : yes 10:29:03 <tim> : ok, let's do it then 10:29:28 <tim> : we don't need another pre-release for that, do we? 10:29:47 <wtay> : don't think so 10:31:50 <wtay> : hm, it might not be right when the start is not 0 10:34:52 <philn>: wtay_: start of the segment? 10:35:12 <wtay> : philn-tp, say you play from 1 second to 3 seconds in reverse 10:35:25 <wtay> : philn-tp, the position in EOS is then 1 second and not 0 10:36:45 <wtay> : philn-tp, maybe last = time is better 10:37:14 <philn>: wtay_: ok, trying that here 10:38:18 <wtay> : also those max and min calls should be different for reverse, I think 10:42:56 <philn>: wtay_: doesn't work :( i play from 0.3 to 0 and position at EOS is 0.3 seconds 10:43:57 <wtay> : let me see.. 10:46:05 <philn>: btw this issue seems to occur with pulsesink only, at least i couldn't reproduce it with fakesink 10:48:06 <wtay> : philn-tp, it does not make sense.. let me try the testapp 10:50:56 <wtay> : philn-tp, how did you test? seems to work for me 10:56:58 <philn>: wtay_: using that python script i attached in the bug... ok well something seems wrong in my setup now, i see in the logs the position query returns 0 indeed with your proposed change 10:57:19 <wtay> : philn-tp, you test app is not very reliable 10:57:39 <wtay> : philn-tp, the easiest is to to to PAUSED, wait for preroll, then seek with -1 and the go to PLAYING 10:59:43 <philn>: wtay_: hum ok trying that here 11:18:42 <philn>: wtay_: ok figured it out, i was doing the query in the wrong format... and the conversion to TIME was failing ;) 11:19:11 <philn>: i'll update the patch 11:19:15 <wtay> : philn-tp, it all seems reasonable to me without any patches 11:19:42 <wtay> : philn-tp, but with the patch, a bug in qtdemux is exposed 11:32:24 <philn>: wtay_: well if qtdemux sends a wrong segment it seems normal that the sink reports a wrong position without the patch no? 11:33:01 <wtay> : philn-tp, it reports a more correct position without the patch 11:46:49 <philn>: wtay_: so you think that bug is invalid? sorry but i'm getting lost now 11:48:40 <wtay> : philn-tp, I'm saying that the position reporting can be improved a little more 11:53:10 <philn>: wtay_: you mean take in account the negative rate with those MAX / MIN calls? 11:54:42 <wtay> : philn-tp, I think so, haven't looked closely. Also the bin should aggregate the min of all returned values etc. 11:57:42 <philn>: ok, that sounds like more work indeed :/
<__tim> wtay_, so tell me, what was the verdict on philn's basesink patch now? Yesterday we first talked about putting it in, but then you had that whole discussion with philn about how it's not really correct and reporting without the patch is more correct etc. <__tim> so now I'm confused :-) <wtay_> __tim, it should wait IMO <wtay_> most files I tested work fine without the patch <wingo-pi> wtay should have a robe and a powdered wig <__tim> he should! <wtay_> __tim, and with the patch quicktime fails because of a bug in qtdemux <__tim> right <wtay_> it could use some more improvements but I think that should be for the next release
Ok and what now? Push after next release? Change something (what?)? :)
Are there some more details on the qtdemux problem? Is it fixed now, how can it be reproduced?
Comment on attachment 178717 [details] [review] basesink: return null position on EOS during reverse playback. From my understanding this patch does the right thing? But shouldn't the last seen position in reverse playback be the segment stop position?
Is there still something necessary here?
(In reply to comment #11) > Is there still something necessary here? Yes, the issue is still there I'm afraid. I'll rework the patch and check the qtdemux issue Wim mentioned.
Created attachment 235920 [details] Updated test case
commit 34eea4d5f27f430b475bd04de5ed90bee9fceb68 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Sun Mar 31 12:55:33 2013 +0200 streamsynchronizer: update position for reverse When doing reverse playback the positino advances from timestamp_end to timestamp.
This is probably fixed now with Wim's commit? Please reopen or create a new bug if this is still a problem in 1.14 or newer