GNOME Bugzilla – Bug 733235
decodebin: Handle the multi-queue size flexible
Last modified: 2017-02-01 14:43:28 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.
What kind of file are you seeing this with ? Can you provide a link to one such file ?
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.
This can be related to https://bugzilla.gnome.org/show_bug.cgi?id=729360
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.
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.
Is this related to https://bugzilla.gnome.org/show_bug.cgi?id=744308 ?
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.
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?
Yes, it was mpegts. I have 4 problem files and all resolution is 3480x2160(same with id=729360). I will test the sintel movie.
Created attachment 300250 [details] [review] Handle the preroll multi-queue size Handle the preroll multi-queue size Rebase the patch based on master.
Created attachment 300251 [details] [review] Handle the multi-queue size flexible Rebase the patch based on master.
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.
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
Created attachment 304227 [details] [review] decodebin: Handle the preroll multi-queue size rebase the patch.
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.
(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.
> 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?
@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.
(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.
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.
@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?
> 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.
Created attachment 308430 [details] [review] decodebin: Handle the preroll multi-queue size decodebin: Handle the preroll multi-queue size rebase and merge the patch.
Created attachment 308520 [details] [review] decodebin: Handle the preroll multi-queue size decodebin: Handle the preroll multi-queue size rebase and merge the patch.
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.
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.
(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.
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'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.
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.
(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.
(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?
(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 :)
(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?
@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.
(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.
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.
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
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
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
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?
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.
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.
Created attachment 309484 [details] [review] decodebin: If extra buffers are going to be required, we're still prerolling
(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.
(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 :)
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).
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.
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.
What's your testcase where increasing the limit makes everything slower?
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.
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.
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.
Created attachment 310369 [details] [review] decodebin: Consider chains complete and deadend if the multiqueue of them had an overrun
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.
(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.
(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.
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.
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 :)
(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. :)
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.
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.
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.
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.
In decodebin3 multiqueue is not longer used for buffering (it should be handled upstream of decodebin3). So yes, it's fixed there :)
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).
Should probably mark this as a duplicate of the decodebin3 bug then?
Yes. Closing and marking as duplicate of decodebin3. *** This bug has been marked as a duplicate of bug 758960 ***