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 733235 - decodebin: Handle the multi-queue size flexible
decodebin: Handle the multi-queue size flexible
Status: RESOLVED DUPLICATE of bug 758960
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.x
Other Linux
: Normal critical
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-16 04:37 UTC by Myoungsun Lee
Modified: 2017-02-01 14:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Handle the multi-queue size (4.23 KB, patch)
2014-07-16 04:37 UTC, Myoungsun Lee
none Details | Review
Handle the preroll multi-queue size (4.24 KB, patch)
2014-07-25 02:00 UTC, Myoungsun Lee
none Details | Review
Handle the preroll multi-queue size (4.16 KB, patch)
2015-03-25 04:37 UTC, Myoungsun Lee
none Details | Review
Handle the multi-queue size flexible (4.21 KB, patch)
2015-03-25 04:45 UTC, Myoungsun Lee
none Details | Review
Add log and compare before with after (302.50 KB, application/zip)
2015-05-29 07:13 UTC, Myoungsun Lee
  Details
decodebin: Handle the preroll multi-queue size (4.70 KB, patch)
2015-05-29 07:46 UTC, Myoungsun Lee
none Details | Review
set extra size to mq if decode chain is not exposed yet (1.28 KB, patch)
2015-07-21 08:30 UTC, Justin Kim
none Details | Review
decodebin: Handle the preroll multi-queue size (5.42 KB, patch)
2015-07-30 01:47 UTC, Myoungsun Lee
none Details | Review
decodebin: Handle the preroll multi-queue size (5.42 KB, patch)
2015-07-31 08:25 UTC, Myoungsun Lee
none Details | Review
decodebin: Handle the preroll multi-queue size (5.82 KB, patch)
2015-08-11 05:59 UTC, Myoungsun Lee
none Details | Review
decodebin: Handle the preroll multi-queue size (6.17 KB, patch)
2015-08-18 15:45 UTC, Sebastian Dröge (slomo)
none Details | Review
decodebin: Store extra_buffer_required per group, not globally (8.23 KB, patch)
2015-08-18 15:46 UTC, Sebastian Dröge (slomo)
none Details | Review
decodebin: If extra buffers are going to be required, we're still prerolling (3.76 KB, patch)
2015-08-18 15:46 UTC, Sebastian Dröge (slomo)
none Details | Review
decodebin: Consider chains complete and deadend if the multiqueue of them had an overrun (938 bytes, patch)
2015-08-31 15:33 UTC, Sebastian Dröge (slomo)
none Details | Review
decodebin: Only switch to "playing" buffering mode once we exposed pads (3.51 KB, patch)
2015-08-31 15:34 UTC, Sebastian Dröge (slomo)
none Details | Review

Description Myoungsun Lee 2014-07-16 04:37:40 UTC
Created attachment 280774 [details] [review]
Handle the multi-queue size

Overview:
The decodebin temporarily increases the multiqueue size until 'no-more-pads' callback is called.

Steps to Reproduce:
play the 4K videos.

Actual Results:
There is no audio or subtitle.
We can see only video.

Expected Results:
The video, audio, text data in file play well.

Additional Information:
If the file has a long-term audio/video/text data, then the multiqueue reachs the max buffer size before all pads are added. It causes abnormal file playing.
The decodebin needs to handle the queue size when the multiqueue is full.
Comment 1 Edward Hervey 2014-07-16 06:21:12 UTC
What kind of file are you seeing this with ? Can you provide a link to one such file ?
Comment 2 Myoungsun Lee 2014-07-16 08:03:42 UTC
I have the file just local. 
There is no link for streaming.

The 4K resolution video is reproduced well.
I tested UHD file(3,840×2,160) and the codec is HEVC.
Comment 3 Thiago Sousa Santos 2014-07-16 13:35:22 UTC
This can be related to https://bugzilla.gnome.org/show_bug.cgi?id=729360
Comment 4 Myoungsun Lee 2014-07-17 00:07:03 UTC
Yeah, The solution concept and problem is same.
This patch is about only pre-roll status, not include during playing status.

Bug 729360 includes "playing status". So I think we can resolve it too, by applying this patch.
Comment 5 Myoungsun Lee 2014-07-25 02:00:19 UTC
Created attachment 281652 [details] [review]
Handle the preroll multi-queue size

The decodebin needs to handle the queue size when the multiqueue is full.
- The max-preroll extra buffer size is setting 8MB. We can use total pre-roll buffer 10MB.
- The extra buffer size that can be added is fixed. Therefore only first overrun callback can handle multiqueue size before no-more-pad callback.
Comment 6 Sebastian Dröge (slomo) 2015-03-23 09:42:00 UTC
Is this related to https://bugzilla.gnome.org/show_bug.cgi?id=744308 ?
Comment 7 Myoungsun Lee 2015-03-24 08:08:03 UTC
Yes, I think it related a little.

In https://bugzilla.gnome.org/show_bug.cgi?id=744308, it adds the logic to handle more detail buffering control and it related in PLAYING status pipeline.

In this patch, it can resolve queue overrun problem before the demux element did not yet called their no_more_pads signal(PAUSED status, it means pre-roll).
If the queue is full before decodebin did not receive no_more_pads signal, then pipeline is just waiting for exhausting the queue. 
In this situation, the queue can not be exhausted, because the pipeline cannot goes on PLAYING status. So it seems like infinite loading.

This patch can resolve it by re-setting the buffer size in pre-roll status.
Comment 8 Thiago Sousa Santos 2015-03-24 15:46:39 UTC
What container are you using? Is this perhaps mpegts?

Can you reproduce with other high resolution files such as the sintel movie pointed at https://bugzilla.gnome.org/show_bug.cgi?id=729360?
Comment 9 Myoungsun Lee 2015-03-25 01:31:46 UTC
Yes, it was mpegts. 
I have 4 problem files and all resolution is 3480x2160(same with id=729360).

I will test the sintel movie.
Comment 10 Myoungsun Lee 2015-03-25 04:37:04 UTC
Created attachment 300250 [details] [review]
Handle the preroll multi-queue size

Handle the preroll multi-queue size 

Rebase the patch based on master.
Comment 11 Myoungsun Lee 2015-03-25 04:45:52 UTC
Created attachment 300251 [details] [review]
Handle the multi-queue size flexible

Rebase the patch based on master.
Comment 12 Myoungsun Lee 2015-03-26 01:18:22 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=729360?
Bug is not resolved this patch.

This patch is for pre-roll stage(PAUSED status), not include the PLAYING status and Buffering.

I tested id=729360 bug and it was normally proceed to PLAYING state.
But after it changed to PLAYING state, pipeline started buffering.

I think it's better to focus on queue handle in PLAYING state about that bug.
Comment 13 Myoungsun Lee 2015-05-29 07:13:12 UTC
Created attachment 304217 [details]
Add log and compare before with after

I attached the log and compare the problem before with after applying patch.
This file has huge resolution(4K file) and size, TS container.
If the overrun is occurred before no-more-pad callback, then the buffer is set "playing' mode. This pipeline didn't add audio pad, but it starts to play.
So I can't hear the audio.  
=> resolution : "decodebin" has to wait until it receives no-more-pad signal from demux even though multiqueue occurred "overrun".
I attached full log with "decodebin:5,tsdemux:4"

**"decproxy" means "decoder" in these logs.


<The problem log>
0:00:00.092669041  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:2551:pad_added_cb:<tsdemux0:video_0101> pad added, chain:0x15e688
0:00:00.093359125  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.093375666  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 2097152 bytes, 0 buffers, 0
0:00:00.131184958  3628 0x71f13b20 DEBUG              decodebin gstdecodebin2.c:2551:pad_added_cb:<capsfilter0:src> pad added, chain:0x72914990
0:00:00.144234583  3628 0x71f13b20 DEBUG              decodebin gstdecodebin2.c:2551:pad_added_cb:<decproxy0:src> pad added, chain:0x72914990
0:00:00.360101250  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3037:multi_queue_overrun_cb:<decodebin0> Setting group 0x71f21878 multiqueue to 'playing' buffering mode
0:00:00.360144708  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.360161917  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 4194304 bytes, 5 buffers, 0
0:00:00.360295958  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.360309708  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 4194304 bytes, 5 buffers, 0
0:00:00.528230500  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:2551:pad_added_cb:<tsdemux0:audio_0102> pad added, chain:0x15e688
0:00:00.528474083  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.528494292  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 4194304 bytes, 5 buffers, 0
0:00:00.528685625  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.528699375  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 4194304 bytes, 5 buffers, 0
0:00:00.528851667  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3154:decodebin_set_queue_size:<multiqueue0> use buffering 0
0:00:00.528865792  3628 0x7291dc00 DEBUG              decodebin gstdecodebin2.c:3183:decodebin_set_queue_size:<multiqueue0> setting limits 4194304 bytes, 5 buffers, 0



<Apply this patch>
0:00:00.764665875  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<tsdemux0:video_0101> pad added, chain:0x1d2290
0:00:00.765359334  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3177:decodebin_set_queue_size:<multiqueue0> use buffering 0, add extra buffer size mode 0
0:00:00.765380125  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3212:decodebin_set_queue_size:<multiqueue0> setting limits 2097152 bytes, 0 buffers, 0
0:00:00.800621542  3774 0x71f13b20 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<capsfilter0:src> pad added, chain:0x72913178
0:00:00.812082417  3774 0x71f13b20 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<decproxy0:src> pad added, chain:0x72913178
0:00:01.039390959  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3049:multi_queue_overrun_cb:<decodebin0> Setting group 0x72923918 multiqueue to 'extra_buffer_required’ mode
0:00:01.039426626  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3177:decodebin_set_queue_size:<multiqueue0> use buffering 0, add extra buffer size mode 1
0:00:01.039447501  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3212:decodebin_set_queue_size:<multiqueue0> setting limits 10485760 bytes, 8388608 buffers, 0
0:00:01.064445126  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<tsdemux0:audio_0102> pad added, chain:0x1d2290
0:00:01.071680751  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3177:decodebin_set_queue_size:<multiqueue0> use buffering 0, add extra buffer size mode 0
0:00:01.071697959  3774 0x7291aa00 DEBUG              decodebin gstdecodebin2.c:3212:decodebin_set_queue_size:<multiqueue0> setting limits 8388608 bytes, 5 buffers, 0
0:00:01.078827084  3774 0x71f10980 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<aacparse0:src> pad added, chain:0x729131c0
0:00:01.090991459  3774 0x71f10980 DEBUG              decodebin gstdecodebin2.c:2560:pad_added_cb:<decproxy1:src> pad added, chain:0x729131c0
0:00:01.092125917  3774 0x71f10980 DEBUG              decodebin gstdecodebin2.c:3177:decodebin_set_queue_size:<multiqueue0> use buffering 0, add extra buffer size mode 0
0:00:01.092150001  3774 0x71f10980 DEBUG              decodebin gstdecodebin2.c:3212:decodebin_set_queue_size:<multiqueue0> setting limits 8388608 bytes, 5 buffers, 0
Comment 14 Myoungsun Lee 2015-05-29 07:46:02 UTC
Created attachment 304227 [details] [review]
decodebin: Handle the preroll multi-queue size

rebase the patch.
Comment 15 Justin Kim 2015-07-20 01:29:23 UTC
As I understand, this problem still reproduces in master because there's no changes in building element chain sequence of decodebin.

The mainly problem is that multiqueue is filled before demux like element sends 'no-more-pad' signal so this causes pipeline stalling in preroll status.

In Myoungsun's code, the size of mq grows certain amount of MB when it comes to overrun before getting 'no-more-pad' from demux. However, I am curious to know whether constant growing without notification is the best.
Comment 16 Nicolas Dufresne (ndufresne) 2015-07-20 13:26:14 UTC
(In reply to Justin J. Kim from comment #15)
> In Myoungsun's code, the size of mq grows certain amount of MB when it comes
> to overrun before getting 'no-more-pad' from demux. However, I am curious to
> know whether constant growing without notification is the best.

The brings a good idea to the table. We could have configured limit (update to todays flavour, at least 4K). And have a mechanism (like a signal), in which application could allow the queue to grow bigger. Application can take into considering it's own limitation.
Comment 17 Justin Kim 2015-07-20 13:36:38 UTC
> And have a mechanism (like a signal), in which application could allow the queue to grow bigger.

I'm testing some codes to explain a draft idea. :) but you already mentioned.

1. add 'more-memory-for-preroll' to decodebin

 gint64 more-memory-for-preroll (GstElement *decodebin, gint64 growing_size)

2. if there's no signal handler, decodebin sets mq size as predefined size (like Myungsun's codes)

3. if user adds signal handler, decodebin uses the return value of signal.
   (-1: immediately fail, 0: managed by application, n: growing size)

How about my proposal?
Comment 18 Myoungsun Lee 2015-07-21 01:38:43 UTC
@Sebastian Dröge (slomo)
I will share the media file about this.
It is reproduced this problem. 

The file has a security policy. So I will send by your mail.
Comment 19 Myoungsun Lee 2015-07-21 02:21:25 UTC
(In reply to Justin J. Kim from comment #17)
> > And have a mechanism (like a signal), in which application could allow the queue to grow bigger.
> 
> I'm testing some codes to explain a draft idea. :) but you already mentioned.
> 
> 1. add 'more-memory-for-preroll' to decodebin
> 
>  gint64 more-memory-for-preroll (GstElement *decodebin, gint64 growing_size)
> 
> 2. if there's no signal handler, decodebin sets mq size as predefined size
> (like Myungsun's codes)
> 
> 3. if user adds signal handler, decodebin uses the return value of signal.
>    (-1: immediately fail, 0: managed by application, n: growing size)
> 
> How about my proposal?

I think it is good to handle the multiqueue size. I haven't thought to the mq size control. 
If any user or media need to resize the mq and control the state then your proposal will be useful. Also, it may use to prevent the memory leakage.
Comment 20 Justin Kim 2015-07-21 08:30:58 UTC
Created attachment 307802 [details] [review]
set extra size to mq if decode chain is not exposed yet

@Myungsun, if audio data position is so far in push mode, the pipeline is stalled. But in your code, you just check 'no-more-pad' flag. IMHO, we should check if decode chain is exposed also.
please, review my additional patch.
Comment 21 Myoungsun Lee 2015-07-28 01:35:22 UTC
@Justin

Your patch can resolve the interleaving file problem in streaming case. 
But is it OK to change the buffer size based on AUTO_PREROLL_SIZE_BYTES ?
I think your patch is about PLAYING state, and the *_SIZE_BYTES are different between preroll and playing state.

Also, is it OK if the queue size doesn't back to original?
Comment 22 Justin Kim 2015-07-28 04:26:28 UTC
> But is it OK to change the buffer size based on AUTO_PREROLL_SIZE_BYTES ?

as far as I am concerned, it doesn't matter because increased mq size doesn't mean to consume memory immediately. (it just says a kind of memory capability, it's not 'malloc')

However, my additional patch is not working like you've mentioned. In your patch, the mq size will be restore if the state goes PLAYING, so my additional patch will do, too.
Comment 23 Myoungsun Lee 2015-07-30 01:47:44 UTC
Created attachment 308430 [details] [review]
decodebin: Handle the preroll multi-queue size

decodebin: Handle the preroll multi-queue size

rebase and merge the patch.
Comment 24 Myoungsun Lee 2015-07-31 08:25:38 UTC
Created attachment 308520 [details] [review]
decodebin: Handle the preroll multi-queue size

decodebin: Handle the preroll multi-queue size

rebase and merge the patch.
Comment 25 Sebastian Dröge (slomo) 2015-08-10 11:05:24 UTC
Review of attachment 308520 [details] [review]:

Please describe in the commit message also *what* you do to fix that, and *how* that is fixing this case and *why* it is the right thing to do :) Additional to the problem description.

::: gst/playback/gstdecodebin2.c
@@ +3373,3 @@
+        "'playing' buffering mode", group);
+    group->overrun = TRUE;
+    dbin->extra_buffer_required = FALSE;

This doesn't seem 100% correct. Consider the case where you set extra_buffer_required=TRUE before. It will then buffer a bit more again. And then at some point might overrun again. You would then set the limits to small again, without the extra buffer space, making things even worse.

@@ +3391,3 @@
+      dbin->extra_buffer_required = TRUE;
+      decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
+          (group->parent ? group->parent->seekable : TRUE));

Now you have two else cases in multiqueue_overrun_cb() that set the queue size. It would be good to have the queue size setting all in a single place to keep code a bit more readable.
Comment 26 Myoungsun Lee 2015-08-11 05:59:35 UTC
Created attachment 309046 [details] [review]
decodebin: Handle the preroll multi-queue size

Overview:
There are some of interleaved streams which has long-term location of audio data. 
It mean the audio data is located far away more than multiqueue size. 
In this case, because of multiqueue overrun, the pipeline is stopped.
To prevent hanging-like state, the decodebin needs to handle the queue size.

Caused:
The multiqueue size is not enough, the pipeline will stay being stalled status 
and decodebin cannot complete to build decode chain. 
In this issue file, decodebin did not receive no_more_pads signal or audio data yet.

Steps to Reproduce:
play the high-resolution(4K file) files or some streaming media(push mode). 

Actual Results:
There is no audio or subtitle.
We can see only video or infinite loading.

Resolution: 
Decodebin detect this problem, and add extra buffer size to multiqueue.
The multiqueue is larger than before, the next data can be pushed the downstream element.

Additional Information:
The max-preroll extra buffer size is set 8MB.
We can use total pre-roll buffer 10MB.
Only first overrun callback can handle multiqueue size.
Comment 27 Myoungsun Lee 2015-08-11 06:33:11 UTC
(In reply to Sebastian Dröge (slomo) from comment #25)


> ::: gst/playback/gstdecodebin2.c
> @@ +3373,3 @@
> +        "'playing' buffering mode", group);
> +    group->overrun = TRUE;
> +    dbin->extra_buffer_required = FALSE;
> 
> This doesn't seem 100% correct. Consider the case where you set
> extra_buffer_required=TRUE before. It will then buffer a bit more again. And
> then at some point might overrun again. You would then set the limits to
> small again, without the extra buffer space, making things even worse.

This patch supported only multi-queue size of pre-roll state. 
It is fixed the problem which the pipeline is stopped in pre-roll state.

I think the code of your review is about PLAYING buffer size, right?
If the overrun is occurred in PLAYING state, then decodebin does nothing originally.
So I think that it is not necessary the extra buffer in PLAYING state. 

Also I uploaded new patch.
Comment 28 Sebastian Dröge (slomo) 2015-08-11 10:08:48 UTC
No, my comment was about the case where you are *not* in PLAYING yet. You overrun, set extra buffers, now the multiqueue is not overrun anymore, at some point it might overrun again before going to PLAYING and you would then go into that if-branch. There you would set less buffering again, making the second overrun even worse.
Comment 29 Olivier Crête 2015-08-12 00:36:38 UTC
I'm also hitting this with a couple of valid 1080i mpeg-ts streams, and this stream play without audio in 1.4, and now it just blocks, so it's a bit of a regression. So if possible, we should try to get something into 1.6.
Comment 30 Justin Kim 2015-08-12 02:15:24 UTC
Review of attachment 309046 [details] [review]:

::: gst/playback/gstdecodebin2.c
@@ +3395,3 @@
+    decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
+        (group->parent ? group->parent->seekable : TRUE));
+  }

Is there any specific reason this statement here (out of EXPOSE_LOCK)?
For thread-safety, it would be better to be done before calling EXPOSE_UNLOCK.
Comment 31 Myoungsun Lee 2015-08-13 05:07:38 UTC
(In reply to Sebastian Dröge (slomo) from comment #28)
> No, my comment was about the case where you are *not* in PLAYING yet. You
> overrun, set extra buffers, now the multiqueue is not overrun anymore, at
> some point it might overrun again before going to PLAYING and you would then
> go into that if-branch. There you would set less buffering again, making the
> second overrun even worse.

I got it :). I'll think about it, and share some solutions or another logic. Thank you.

--------------
(In reply to Justin J. Kim from comment #30)
> Review of attachment 309046 [details] [review] [review]:
> 
> ::: gst/playback/gstdecodebin2.c
> @@ +3395,3 @@
> +    decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
> +        (group->parent ? group->parent->seekable : TRUE));
> +  }
> 
> Is there any specific reason this statement here (out of EXPOSE_LOCK)?
> For thread-safety, it would be better to be done before calling
> EXPOSE_UNLOCK.
There is no reason. Maybe I missed that point, I will fix it. Thank you.
Comment 32 Myoungsun Lee 2015-08-13 05:08:43 UTC
(In reply to Olivier Crête from comment #29)
> I'm also hitting this with a couple of valid 1080i mpeg-ts streams, and this
> stream play without audio in 1.4, and now it just blocks, so it's a bit of a
> regression. So if possible, we should try to get something into 1.6.

If you apply this patch, then is it resolved the problem?
Comment 33 Sebastian Dröge (slomo) 2015-08-13 10:51:19 UTC
(In reply to Myoungsun Lee from comment #31)

> (In reply to Justin J. Kim from comment #30)
> > Review of attachment 309046 [details] [review] [review] [review]:
> > 
> > ::: gst/playback/gstdecodebin2.c
> > @@ +3395,3 @@
> > +    decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
> > +        (group->parent ? group->parent->seekable : TRUE));
> > +  }
> > 
> > Is there any specific reason this statement here (out of EXPOSE_LOCK)?
> > For thread-safety, it would be better to be done before calling
> > EXPOSE_UNLOCK.
> There is no reason. Maybe I missed that point, I will fix it. Thank you.

Reason might be that setting properties emits signals (the notify::* signals), someone might catch them and call back into decodebin code... then causing a deadlock. Just a guess, it might also be correct to just move it inside the lock :)
Comment 34 Olivier Crête 2015-08-13 15:19:38 UTC
(In reply to Myoungsun Lee from comment #32)
> If you apply this patch, then is it resolved the problem?

Yes, it just works! I'd love to have something more dynamic than just adding a magic 8MB, but this seems to work for real use-cases, so I'd favour merging it for now. I assume you've tested with 4K content? Did you have a chance to try 8K content?
Comment 35 Myoungsun Lee 2015-08-18 04:33:28 UTC
@Sebastian Dröge (slomo), @Justin J. Kim
multi_queue_overrun_cb() did not lock when it sets queue size, so I just added it outside lock. I tested it inside the lock, it works well. :)
Anyway, I still concerned about any unexpected deadlock as Sebastian mentioned.

@Olivier
I'm very glad that this patch helps problem solving.
I don't have any 8K contents, so I cannot tested it yet.
Comment 36 Sebastian Dröge (slomo) 2015-08-18 08:36:22 UTC
(In reply to Myoungsun Lee from comment #35)
> @Sebastian Dröge (slomo), @Justin J. Kim
> multi_queue_overrun_cb() did not lock when it sets queue size, so I just
> added it outside lock. I tested it inside the lock, it works well. :)
> Anyway, I still concerned about any unexpected deadlock as Sebastian
> mentioned.

I think that's going to cause problems, can you check with "git blame" around the lines of the locks what added them? I remember something having to be fixed for the locking to prevent deadlocks.
Comment 37 Sebastian Dröge (slomo) 2015-08-18 10:06:35 UTC
From looking at the code, adding the locking everywhere seems safe. It's not only multiqueue_overun_cb though, but also gst_decode_group_new().

I would still prefer to have the locking outside decodebin_set_queue_size() instead of inside that function.
Comment 38 Sebastian Dröge (slomo) 2015-08-18 10:09:58 UTC
Review of attachment 309046 [details] [review]:

::: gst/playback/gstdecodebin2.c
@@ +3370,3 @@
+    dbin->extra_buffer_required = FALSE;
+    decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
+        (group->parent ? group->parent->seekable : TRUE));

I meant the CHAIN_LOCK here btw, not the EXPOSE_LOCK

@@ +3395,3 @@
+    decodebin_set_queue_size (group->dbin, group->multiqueue, FALSE,
+        (group->parent ? group->parent->seekable : TRUE));
+  }

EXPOSE_LOCK seems unrelated to the queue size, CHAIN_LOCK seems to be needed though
Comment 39 Sebastian Dröge (slomo) 2015-08-18 11:11:19 UTC
Review of attachment 309046 [details] [review]:

Some more things, I'm fixing them up now and merge everything.

::: gst/playback/gstdecodebin2.c
@@ +1102,3 @@
   decode_bin->connection_speed = DEFAULT_CONNECTION_SPEED;
+
+  decode_bin->extra_buffer_required = FALSE;

This also needs to be reset to FALSE whenever decodebin goes back to READY

@@ +3559,3 @@
+  GST_DEBUG_OBJECT (multiqueue,
+      "use buffering %d, add extra buffer size mode %d", use_buffering,
+      dbin->extra_buffer_required);

Why not have this here:
gint extra_bytes = (dbin->extra_buffer_required ? DEFAULT_EXTRA_SIZE_BUFFERS_BYTES : 0);
gint extra_buffers = ...;

and then just add those values below instead of adding a new if-case?

@@ +3577,3 @@
+  } else if (dbin->extra_buffer_required) {
+    max_bytes = AUTO_PREROLL_SIZE_BYTES + DEFAULT_EXTRA_SIZE_BUFFERS_BYTES;
+    max_buffers = AUTO_PREROLL_SIZE_BUFFERS + DEFAULT_EXTRA_SIZE_BUFFERS_BYTES;

You're adding buffers and bytes here
Comment 40 Sebastian Dröge (slomo) 2015-08-18 12:22:43 UTC
Please test if these changes work fine for you


commit a3b24f0241bd55a005a072ba8ddcd53e0fdbf827
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Aug 18 15:16:25 2015 +0300

    decodebin: If extra buffers are going to be required, we're still prerolling

commit 1ea81114ea6bd48b581f19002018680933aa7a12
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Aug 18 15:01:33 2015 +0300

    decodebin: Store extra_buffer_required per group, not globally
    
    It's only relevant for each group, and by storing it in the group
    we have locking and everything else like for the other buffering-related
    variables. Locking looks a bit fishy still, but it was like that for a long
    time already so shouldn't be worse than before.

commit 5c8ef0ea05123506dfc35c70c8b165bca7435dad
Author: Myoungsun Lee <ohmygod0327@gmail.com>
Date:   Thu Jul 30 10:33:25 2015 +0900

    decodebin: Handle the preroll multi-queue size
    
    Overview:
    There are some of interleaved streams which has long-term location of audio data.
    It mean the audio data is located far away more than multiqueue size.
    In this case, because of multiqueue overrun, the pipeline is stopped.
    To prevent hanging-like state, the decodebin needs to handle the queue size.
    
    Caused:
    The multiqueue size is not enough, the pipeline will stay being stalled status
    and decodebin cannot complete to build decode chain.
    In this issue file, decodebin did not receive no_more_pads signal or audio data yet.
    
    Steps to Reproduce:
    play the high-resolution(4K file) files or some streaming media(push mode).
    
    Actual Results:
    There is no audio or subtitle.
    We can see only video or infinite loading.
    
    Resolution:
    Decodebin detect this problem, and add extra buffer size to multiqueue.
    The multiqueue is larger than before, the next data can be pushed the downstream element.
    
    Additional Information:
    The max-preroll extra buffer size is set 8MB.
    We can use total pre-roll buffer 10MB.
    Only first overrun callback can handle multiqueue size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733235
Comment 41 Sebastian Dröge (slomo) 2015-08-18 15:44:50 UTC
This causes the playbin-complex test in gst-plugins-base to fail.


Problem here is that overrun is also emitted on EOS, so in those tests we overrun at EOS but never expose any pads because after EOS nothing else comes anymore. And no-more-pads does not happen in this test.

Actually this patch is also not great in general for demuxers that don't emit no-more-pads. They will all get 8MB more of buffering, and then it takes 8MB more before the streams are exposed.

Reverting for now, and attaching the patches to the bug. Does anybody have a better idea how to fix this?
Comment 42 Sebastian Dröge (slomo) 2015-08-18 15:45:56 UTC
Created attachment 309482 [details] [review]
decodebin: Handle the preroll multi-queue size

Overview:
There are some of interleaved streams which has long-term location of audio data.
It mean the audio data is located far away more than multiqueue size.
In this case, because of multiqueue overrun, the pipeline is stopped.
To prevent hanging-like state, the decodebin needs to handle the queue size.

Caused:
The multiqueue size is not enough, the pipeline will stay being stalled status
and decodebin cannot complete to build decode chain.
In this issue file, decodebin did not receive no_more_pads signal or audio data yet.

Steps to Reproduce:
play the high-resolution(4K file) files or some streaming media(push mode).

Actual Results:
There is no audio or subtitle.
We can see only video or infinite loading.

Resolution:
Decodebin detect this problem, and add extra buffer size to multiqueue.
The multiqueue is larger than before, the next data can be pushed the downstream element.

Additional Information:
The max-preroll extra buffer size is set 8MB.
We can use total pre-roll buffer 10MB.
Only first overrun callback can handle multiqueue size.
Comment 43 Sebastian Dröge (slomo) 2015-08-18 15:46:06 UTC
Created attachment 309483 [details] [review]
decodebin: Store extra_buffer_required per group, not globally

It's only relevant for each group, and by storing it in the group
we have locking and everything else like for the other buffering-related
variables. Locking looks a bit fishy still, but it was like that for a long
time already so shouldn't be worse than before.
Comment 44 Sebastian Dröge (slomo) 2015-08-18 15:46:15 UTC
Created attachment 309484 [details] [review]
decodebin: If extra buffers are going to be required, we're still prerolling
Comment 45 Myoungsun Lee 2015-08-19 02:45:32 UTC
(In reply to Sebastian Dröge (slomo) from comment #41)
> This causes the playbin-complex test in gst-plugins-base to fail.
> 
> 
> Problem here is that overrun is also emitted on EOS, so in those tests we
> overrun at EOS but never expose any pads because after EOS nothing else
> comes anymore. And no-more-pads does not happen in this test.
> 
> Actually this patch is also not great in general for demuxers that don't
> emit no-more-pads. They will all get 8MB more of buffering, and then it
> takes 8MB more before the streams are exposed.
> 
> Reverting for now, and attaching the patches to the bug. Does anybody have a
> better idea how to fix this?

I tested it, and I wonder why the EOS comes up before no-more-pad. It could be related in very small file size. 

Anyway, in this patch, I set overrun setting(group->overrun) to FALSE, because I want to make decode-group is not completed. In 4K video case, decode-group have to wait the audio data which is located far from the video data. Therefore decode-group is not completed until decodebin get no-more-pad signal. It means that extra_buffer_required is set FALSE by only no-more-pad signal. 

Your test is very good to fix this patch, also include exception case.
I will think about the solution, too.
Comment 46 Sebastian Dröge (slomo) 2015-08-19 07:16:47 UTC
(In reply to Myoungsun Lee from comment #45)
> (In reply to Sebastian Dröge (slomo) from comment #41)
> > This causes the playbin-complex test in gst-plugins-base to fail.
> > 
> > 
> > Problem here is that overrun is also emitted on EOS, so in those tests we
> > overrun at EOS but never expose any pads because after EOS nothing else
> > comes anymore. And no-more-pads does not happen in this test.
> > 
> > Actually this patch is also not great in general for demuxers that don't
> > emit no-more-pads. They will all get 8MB more of buffering, and then it
> > takes 8MB more before the streams are exposed.
> > 
> > Reverting for now, and attaching the patches to the bug. Does anybody have a
> > better idea how to fix this?
> 
> I tested it, and I wonder why the EOS comes up before no-more-pad. It could
> be related in very small file size. 

demuxers are not required to ever do no-more-pads, you'll have to work properly without no-more-pads and with EOS happening before no-more-pads.

> Anyway, in this patch, I set overrun setting(group->overrun) to FALSE,
> because I want to make decode-group is not completed. In 4K video case,
> decode-group have to wait the audio data which is located far from the video
> data. Therefore decode-group is not completed until decodebin get
> no-more-pad signal. It means that extra_buffer_required is set FALSE by only
> no-more-pad signal. 

Understood, for the case where no-more-pads happens, your patch is definitely going to solve some problems :)
Comment 47 Sebastian Dröge (slomo) 2015-08-20 11:42:48 UTC
Alternatively we could just set the bytes limits much higher. 8MB, 10MB, something like that. Needs to be tested with multipartdemux and other demuxers that don't do no-more-pads though to check if bad things are happening (like prerolling taking multiple times as long).
Comment 48 Sebastian Dröge (slomo) 2015-08-31 14:03:07 UTC
So should we just go with increasing AUTO_PREROLL_SIZE_BYTES to 10MB and be done with it? This should make things work with high-bitrate streams from this decade again.
Comment 49 Justin Kim 2015-08-31 14:29:01 UTC
Although I don't have idea, I'm writing comment just for information.

> So should we just go with increasing AUTO_PREROLL_SIZE_BYTES to 10MB and be done with it?

Perhaps, it will help to solve this problem. But as you mentioned, it will increase prerolling time. Once I set the value as 16MB, almost of test case related to check loading time was worse comparing to the case when the size is 2MB. (but I am not 100% sure if it really affected on that) That's why Myungsun and I were trying to find a certain condition to increase this value.

if decodebin detects high-bitrate, it would be better solution than the previous patches. However, I don't know a general way to catch it.
Comment 50 Sebastian Dröge (slomo) 2015-08-31 14:31:29 UTC
What's your testcase where increasing the limit makes everything slower?
Comment 51 Justin Kim 2015-08-31 14:50:55 UTC
As I remember, most of push mode was affected by the increased size. 
at that time, I got tons of report from QA team after that patch was released. 
but it was a year ago (based on 1.2.x) so I am not sure whether the problem will be reproduced on master.
Comment 52 Sebastian Dröge (slomo) 2015-08-31 14:54:24 UTC
If this is MPEG TS, tsdemux nowadays reports no-more-pads while it didn't in 1.2. That would make a big difference here.
Comment 53 Justin Kim 2015-08-31 15:10:02 UTC
yes! but unfortunately, one thing I remember is sintel movie(mp4) page was included.
http://www.w3.org/2010/05/video/mediaevents.html

I think, it's better to ask loading time change report with increased size patch to Myungsun.
Comment 54 Sebastian Dröge (slomo) 2015-08-31 15:33:56 UTC
Created attachment 310369 [details] [review]
decodebin: Consider chains complete and deadend if the multiqueue of them had an overrun
Comment 55 Sebastian Dröge (slomo) 2015-08-31 15:34:04 UTC
Created attachment 310370 [details] [review]
decodebin: Only switch to "playing" buffering mode once we exposed pads

Not already when we got overrun or no-more-pads, we still have some further
steps to do before we are actually prerolled. Like getting caps for all
streams.
Comment 56 Sebastian Dröge (slomo) 2015-08-31 15:37:39 UTC
(In reply to Sebastian Dröge (slomo) from comment #55)
> Created attachment 310370 [details] [review] [review]
> decodebin: Only switch to "playing" buffering mode once we exposed pads
> 
> Not already when we got overrun or no-more-pads, we still have some further
> steps to do before we are actually prerolled. Like getting caps for all
> streams.

This basically reverts the changes from bug #740689. Problem with those changes is that we switch to "playing" mode before we're actually ready to expose pads. We only expose pads once they all have caps or became deadends.
Comment 57 Sebastian Dröge (slomo) 2015-08-31 15:38:15 UTC
(In reply to Sebastian Dröge (slomo) from comment #54)
> Created attachment 310369 [details] [review] [review]
> decodebin: Consider chains complete and deadend if the multiqueue of them
> had an overrun

This makes streams like the ones discussed here fail completely instead of letting the multiqueue run full and then wait forever.
Comment 58 Sebastian Dröge (slomo) 2015-08-31 15:44:21 UTC
With these two patches, *plus* increasing the preroll limit to 20MB, things actually seem to work.

The deadend patch makes it non-deterministic though. The multiqueue might overrun before the CAPS event is sent downstream from the queue and decodebin decided to expose a pad. Not sure how to handle this best. If we expose directly when overrun happens (that's the deadend patch), then we might fail streams that would otherwise work due to CAPS not being at the end at overrun... if we expose only once CAPS are there for all streams, then we might wait forever in case of overruns.
Comment 59 Sebastian Dröge (slomo) 2015-09-01 10:02:53 UTC
So not sure what to do about this.

Currently we set the limits to the playing limits on no-more-pads/overrun and then wait for caps. Which can easily lead to waiting forever for caps, while we would get caps if we just kept the preroll limits a bit longer. It is also completely non-deterministic as we might or might not get enough data until the point when the playing limits are set thanks to things happening in other threads.

Immediately exposing pads on overrun and ignoring all pads that have no caps yet leads to the problem that it often we don't expose pads just because the caps events did not travel far enough yet downstream of the multiqueue before the multiqueue was running full upstream.

Not waiting for caps would break applications that expect decodebin and friends to have caps immediately on the pads they add. And there are many.

Also the way how things are right now, the overrun case seems mostly useless. It doesn't change anything, pads are still not considered ready to be exposed because of the overrun but we're waiting for caps on all of them.


Independent of that, just increasing the preroll limits does not solve anything unless you're lucky with how timing works. The extra buffering patches in this bug changed timing in a way that made some test streams work relatively reliable, but that's obviously not a real solution :)

I think what should happen regarding the buffering is that there are (higher) preroll limits that are kept until we are actually starting to output data (conflict with bug #740689), and then the new playing limits are based on some minimum value and the current queues fill levels.


All this is relatively intrusive, so not sure if anything will happen here at all before 1.6. Unless someone has a great idea :)
Comment 60 Myoungsun Lee 2015-09-02 01:59:09 UTC
(In reply to Sebastian Dröge (slomo) from comment #58)
=>I also tested it, and it works well. But as your concern, I wonder that just bigger queue-size is the solution. 
I think we check the more conditions to complete the pipeline perfectly. 
 

(In reply to Sebastian Dröge (slomo) from comment #59)
=> I agree with you. We cannot determine the queue size and exposed conditions, just by no-more-pads/overrun.
Now we have no idea about it, so I think one points what we consider.

- How can we control the queue size when overrun happend, before it get the no-more-pads signal?

It seems to be easy but there are many relative things as you said.
So I continuously think the solution, whether it needs more conditions, signals or logics. :)
Comment 61 Sebastian Dröge (slomo) 2015-09-02 18:26:50 UTC
I'm going to keep this as blocker for 1.6.0 for now, but if nobody has any great ideas how to fix this without breaking the other things it will be deferred until 1.8 or so... and for 1.8 someone should rewrite how the buffering stuff works inside decodebin.
Comment 62 Wonchul Lee 2015-09-03 03:33:04 UTC
afaik, what if decodebin can construct chain after switching playing state? (this is another story)
If it is possible, decodebin does not have to wait caps which comes uncertain time.
it just creates related elements dynamically when caps is arriving, playbin either, though not sure feasible.
Comment 63 Sebastian Dröge (slomo) 2015-09-03 07:20:39 UTC
It's part of the decodebin API that it always has caps on the pads it is exposing, so we can't change that before decodebin3. But that's how it should be IMHO, there's no good reason for having decodebin always wait if it knows that it can expose the streams already.

For the current decodebin design it's a requirement though, also because we have no other way of knowing if a stream can be exposed or not other than its caps.
Comment 64 Sebastian Dröge (slomo) 2016-02-17 14:35:53 UTC
Not really a blocker as it's no regression. Still should be fixed somehow, question is if decodebin3 already solves this due to the different buffering it does.
Comment 65 Edward Hervey 2016-02-17 14:37:27 UTC
In decodebin3 multiqueue is not longer used for buffering (it should be handled upstream of decodebin3). So yes, it's fixed there :)
Comment 66 Edward Hervey 2016-02-17 15:16:29 UTC
For clarification, decodebin3 still uses multiqueue, but it's only used to cope with interleaving and decoder buffering requirements (i.e. vbv). This can be handled properly since it deals with timestamped data (parsers before multiqueue, unliked decodebin2).

Other kinds of buffering are handled upstream of decodebin3 (such as in urisourcebin, used by playbin3).
Comment 67 Sebastian Dröge (slomo) 2016-02-17 15:34:00 UTC
Should probably mark this as a duplicate of the decodebin3 bug then?
Comment 68 Edward Hervey 2017-02-01 14:43:28 UTC
Yes. Closing and marking as duplicate of decodebin3.

*** This bug has been marked as a duplicate of bug 758960 ***