GNOME Bugzilla – Bug 764733
qtdemux: Regression in YouTube TV tests in WebKit MSE after fix for #760779
Last modified: 2016-04-21 09:54:33 UTC
In our MSE implementation for WebKitForWayland we found that this patch causes regressions at the tests for YouTube TV. We are using qtdemux to parse the data appended to the SourceBuffer and it seems that they delay in exposing the streams introduced by this patch is an issue. For us the optimal behavior here would be to remove this patch but I guess Seungha Yang wouldn't be very happy with it. Any thoughts? +++ This bug was initially created as a clone of Bug #760779 +++ In case of push mode, qtdemux expose streams after got moov box. We can not guarantee that moov box has sample data such as sample duration and the number of sample in stbl box in fragmented format case. If moov has no sample data, this patch will not expose streams until get the first moof. This patch depends on https://bugzilla.gnome.org/show_bug.cgi?id=760505
(In reply to Xabier Rodríguez Calvar from comment #0) > In our MSE implementation for WebKitForWayland we found that this patch > causes regressions at the tests for YouTube TV. > > We are using qtdemux to parse the data appended to the SourceBuffer and it > seems that they delay in exposing the streams introduced by this patch is an > issue. > > For us the optimal behavior here would be to remove this patch but I guess > Seungha Yang wouldn't be very happy with it. Any thoughts? > > +++ This bug was initially created as a clone of Bug #760779 +++ > > In case of push mode, qtdemux expose streams after got moov box. > We can not guarantee that moov box has sample data such as sample duration > and the number of sample in stbl box in fragmented format case. > If moov has no sample data, this patch will not expose streams > until get the first moof. > This patch depends on https://bugzilla.gnome.org/show_bug.cgi?id=760505 I think revert this patch is reasonable (I'm ok :) ). I'm sorry that MSE case was not fully considered when I prepared the patch. Dear Xabier Rodríguez Calvar, I'll make another solution to cover MSE & normal fragmented case. So, please feel free to request reverting this patch.
We should revert it for 1.8 then, and hopefully find a solution for both cases in 1.9/1.10. Seungha, are you looking into this?
Created attachment 325702 [details] [review] qtdemux: Properly expose streams in mss_mode
(In reply to Sebastian Dröge (slomo) from comment #2) > We should revert it for 1.8 then, and hopefully find a solution for both > cases in 1.9/1.10. Seungha, are you looking into this? I attach one patch. I hope that this patch works properly but I could not verify it due to lack of MSS content. Could anybody let me know about MSS content which I can test?
mss_mode is related with Smooth Streaming, I think. This patch seems unrelated with the regression.
(In reply to Philippe Normand from comment #5) > mss_mode is related with Smooth Streaming, I think. This patch seems > unrelated with the regression. Could you kindly explain more please? What I'm understanding about this issue is as follows - bug #760779 caused delay in expose stream, if moov have no sample information. - MSS mode case, however, exposing stream causes no problem because upstream element provides caps information (as far as I understand)... ==> bug #760779 did not fully consider MSS case so, delay also happened for MSS mode. - Attached patch expects exposing streams with the first moov although the moov has no sample information.
Created attachment 325712 [details] Wrong run log
Created attachment 325713 [details] Right run log
(In reply to Seungha Yang from comment #6) > Could you kindly explain more please? > > What I'm understanding about this issue is as follows > - bug #760779 caused delay in expose stream, if moov have no sample > information. > - MSS mode case, however, exposing stream causes no problem because upstream > element provides caps information (as far as I understand)... > ==> bug #760779 did not fully consider MSS case so, delay also happened for > MSS mode. > - Attached patch expects exposing streams with the first moov although the > moov has no sample information. Phil is right, your patch does not fix the issue. I uploaded a couple of logs, the right one is run after reverting your patch and the wrong one is run before reverting your patch. I hope this can provide some useful information to you.
(In reply to Xabier Rodríguez Calvar from comment #9) > (In reply to Seungha Yang from comment #6) > > Could you kindly explain more please? > > > > What I'm understanding about this issue is as follows > > - bug #760779 caused delay in expose stream, if moov have no sample > > information. > > - MSS mode case, however, exposing stream causes no problem because upstream > > element provides caps information (as far as I understand)... > > ==> bug #760779 did not fully consider MSS case so, delay also happened for > > MSS mode. > > > - Attached patch expects exposing streams with the first moov although the > > moov has no sample information. > > Phil is right, your patch does not fix the issue. I uploaded a couple of > logs, the right one is run after reverting your patch and the wrong one is > run before reverting your patch. > > I hope this can provide some useful information to you. Thanks for providing more information. I perfectly misunderstood this issue.... Uploaded "Wrong run log" shows invalid framerate is set in caps. (the purpose of bug #760779 is exactly fixing it). However, because the delayed exposing stream causes regression, I think we have no choice but to revert bug #760779. Actually, there is no available information to calculate framerate at that moment.
Created attachment 325727 [details] [review] qtdemux: Do not use reliable framerate 1/timescale is not reliable value for framerate. Due to downstream element usually use framerate generated by qtdemux, let it be omitted until the framerate can be reliably calculated.
Newly attached patch is not for solve this problem (to solve this... reverting bug #760779 is the best for me...) But, we need to correct framerate especially my environment. This incorrect framerate is used from this old commit 98acdd1 2006-01-24 gst/qtdemux/qtdemux.c: More coherent framerate setting on caps
So you mean reverting the original patch and using this one instead would be a good fix for the MSE use case and yours?
(In reply to Sebastian Dröge (slomo) from comment #13) > So you mean reverting the original patch and using this one instead would be > a good fix for the MSE use case and yours? That's what it is. What I expect is that Reverting original patch and using the latest patch can solve both problem. I'm not sure whether discussion for the latest patch need to be done here or another page. Anyway, I've never seen that "timescale/1" value is matched to actual framerate.
(In reply to Seungha Yang from comment #3) > Created attachment 325702 [details] [review] [review] > qtdemux: Properly expose streams in mss_mode So this patch is obsolete, and instead the old patch should be reverted and the "qtdemux: Do not use reliable framerate" should be considered? Let's discuss it here, no need for a new bug
Created attachment 325765 [details] [review] qtdemux: Do not use reliable framerate change commit message
(In reply to Sebastian Dröge (slomo) from comment #15) > (In reply to Seungha Yang from comment #3) > > Created attachment 325702 [details] [review] [review] [review] > > qtdemux: Properly expose streams in mss_mode > > So this patch is obsolete, and instead the old patch should be reverted and > the "qtdemux: Do not use reliable framerate" should be considered? > > Let's discuss it here, no need for a new bug Right. The old patch should be reverted. We need to consider "qtdemux: Do not use reliable framerate".
Comment on attachment 325765 [details] [review] qtdemux: Do not use reliable framerate I guess this makes sense, we would otherwise more often than not end up with framerates like 1000/1 (millisecond timescale) and similar
Created attachment 325768 [details] [review] qtdemux: Do not use unreliable framerate change commit message
(In reply to Seungha Yang from comment #19) > Created attachment 325768 [details] [review] [review] > qtdemux: Do not use unreliable framerate > > change commit message I am busy with some other task. I'll try this as soon as I can.
(In reply to Seungha Yang from comment #19) > Created attachment 325768 [details] [review] [review] > qtdemux: Do not use unreliable framerate > > change commit message I reverted the patch that had to be reverted, applied this one and it seems everything works, so I give my green light to this. Thanks!
Ah, this failed to make it into 1.8.1 now. Well, 1.8.2 it is then
Attachment 325768 [details] pushed as cde45a4 - qtdemux: Do not use unreliable framerate