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 764733 - qtdemux: Regression in YouTube TV tests in WebKit MSE after fix for #760779
qtdemux: Regression in YouTube TV tests in WebKit MSE after fix for #760779
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.8.0
Other Linux
: Normal blocker
: 1.8.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-07 14:42 UTC by Xabier Rodríguez Calvar
Modified: 2016-04-21 09:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: Properly expose streams in mss_mode (958 bytes, patch)
2016-04-11 08:06 UTC, Seungha Yang
rejected Details | Review
Wrong run log (49.33 KB, application/octet-stream)
2016-04-11 09:59 UTC, Xabier Rodríguez Calvar
  Details
Right run log (52.15 KB, application/octet-stream)
2016-04-11 09:59 UTC, Xabier Rodríguez Calvar
  Details
qtdemux: Do not use reliable framerate (2.64 KB, patch)
2016-04-11 13:59 UTC, Seungha Yang
none Details | Review
qtdemux: Do not use reliable framerate (2.64 KB, patch)
2016-04-12 07:07 UTC, Seungha Yang
none Details | Review
qtdemux: Do not use unreliable framerate (2.64 KB, patch)
2016-04-12 07:45 UTC, Seungha Yang
committed Details | Review

Description Xabier Rodríguez Calvar 2016-04-07 14:42:11 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
Comment 1 Seungha Yang 2016-04-10 01:05:21 UTC
(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.
Comment 2 Sebastian Dröge (slomo) 2016-04-11 06:18:51 UTC
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?
Comment 3 Seungha Yang 2016-04-11 08:06:33 UTC
Created attachment 325702 [details] [review]
qtdemux: Properly expose streams in mss_mode
Comment 4 Seungha Yang 2016-04-11 08:09:58 UTC
(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?
Comment 5 Philippe Normand 2016-04-11 08:10:43 UTC
mss_mode is related with Smooth Streaming, I think. This patch seems unrelated with the regression.
Comment 6 Seungha Yang 2016-04-11 08:26:58 UTC
(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.
Comment 7 Xabier Rodríguez Calvar 2016-04-11 09:59:16 UTC
Created attachment 325712 [details]
Wrong run log
Comment 8 Xabier Rodríguez Calvar 2016-04-11 09:59:41 UTC
Created attachment 325713 [details]
Right run log
Comment 9 Xabier Rodríguez Calvar 2016-04-11 10:01:34 UTC
(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.
Comment 10 Seungha Yang 2016-04-11 13:02:42 UTC
(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.
Comment 11 Seungha Yang 2016-04-11 13:59:37 UTC
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.
Comment 12 Seungha Yang 2016-04-11 14:01:40 UTC
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
Comment 13 Sebastian Dröge (slomo) 2016-04-12 06:46:00 UTC
So you mean reverting the original patch and using this one instead would be a good fix for the MSE use case and yours?
Comment 14 Seungha Yang 2016-04-12 07:02:56 UTC
(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.
Comment 15 Sebastian Dröge (slomo) 2016-04-12 07:07:49 UTC
(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
Comment 16 Seungha Yang 2016-04-12 07:07:59 UTC
Created attachment 325765 [details] [review]
qtdemux: Do not use reliable framerate

change commit message
Comment 17 Seungha Yang 2016-04-12 07:16:59 UTC
(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 18 Sebastian Dröge (slomo) 2016-04-12 07:36:31 UTC
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
Comment 19 Seungha Yang 2016-04-12 07:45:09 UTC
Created attachment 325768 [details] [review]
qtdemux: Do not use unreliable framerate

change commit message
Comment 20 Xabier Rodríguez Calvar 2016-04-13 09:44:16 UTC
(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.
Comment 21 Xabier Rodríguez Calvar 2016-04-21 09:25:51 UTC
(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!
Comment 22 Sebastian Dröge (slomo) 2016-04-21 09:48:44 UTC
Ah, this failed to make it into 1.8.1 now. Well, 1.8.2 it is then
Comment 23 Sebastian Dröge (slomo) 2016-04-21 09:54:28 UTC
Attachment 325768 [details] pushed as cde45a4 - qtdemux: Do not use unreliable framerate