GNOME Bugzilla – Bug 740458
hlsdemux: typefind might fail if first buffer is too short, causing the whole pipeline to abort
Last modified: 2014-12-24 04:09:45 UTC
On some occasions, the very first buffer that comes to the src_chain in hlsdemux is only 700 odd bytes long. All the subsequent buffers are 4k bytes long. hlsdemux runs gst_type_find_helper_for_buffer() on the data which in turn ends up calling mpeg_ts_type_find and mpeg_ts_type_find needs at least GST_MPEGTS_TYPEFIND_SYNC_SIZE worth of data to do the typefind which is 832 bytes. So, typefind fails and the pipeline errors out. The fix would be that hlsdemux needs to wait for enough bytes before calling typefind on the buffer.
Created attachment 291135 [details] [review] hlsdemux: Wait for sufficient buffer size before running typefind Tested on one of the private urls which didn't work before and it seems to work now with the patch.
Review of attachment 291135 [details] [review]: I'm torn between this solution and moving this to be handled when typefinding really fails. This looks simple and should work but having it handled when the typefind really fails looks more correct. When typefind fails, one would check if the buffer was large enough for typefinding and if not, prepend it to the pending_buffer for when more data is available. Anyway let's wait for a second opinion to see what others think. ::: ext/hls/gsthlsdemux.c @@ +876,3 @@ + /* mpegts typefind fails if size < 4 (208 byte) packets. Round to 1k. */ + if (demux->do_typefind && available < 1024) { Please add a G_UNLIKELY here to hint the compiler that this is unlikely to happen.
Comment on attachment 291135 [details] [review] hlsdemux: Wait for sufficient buffer size before running typefind Thanks for your patch. While you wait for a second opinion: Please fix your commit message so it goes: "hlsdemux: patch description" and add the bug number on another line.
I also think it would be best to move it to when typefinding fails. Given the possible formats, I don't think we need to worry too much about false-positives here (so no need to impose a minimum number of bytes before trying to typefind, although I think that would be fine too).
Then let's go with that. Can you update your patch to have it done when typefinding actually fails? Also include Reynaldo's suggestions on your commit message so we can push it. Let us know if you need more directions. Thanks.
Created attachment 291209 [details] [review] hlsdemux: If typefind fails due to short buffer, save buffer and try again This patch should have the fixes as per the initial review. Special thanks to Thiago for the support! Please review. Thanks.
Review of attachment 291209 [details] [review]: I still have some suggestions below. The commit message is better now but title can be shorter, just use "hlsdemux: typefind might fail if first buffer is too short" Thanks for the quick update! ::: ext/hls/gsthlsdemux.c @@ +923,3 @@ if (G_UNLIKELY (!caps)) { + /* Typefind could fail if buffer is too small. Retry later */ + if (G_UNLIKELY(gst_buffer_get_size(buffer) < 1024)) { Use a larger value here, as this is a corner case we can just give typefind enough data. Quickly browsing through typefinders code shows some requiring 2MB. @@ +926,3 @@ + gst_buffer_prepend_memory(demux->pending_buffer, + gst_buffer_get_all_memory(buffer)); + gst_buffer_unref(buffer); gst_buffer_append does what those 2 lines do.
Hi Thiago, Before I commit a patch again, is this what you had in mind? if (G_UNLIKELY(gst_buffer_get_size(buffer) < (2*1024*1024))) { demux->pending_buffer = gst_buffer_append(buffer, demux->pending_buffer); return GST_FLOW_OK; } Thanks, -Vijay
Exactly.
On a side note, if you're going to update the patch anyway, now that we moved the code out of the "hot path" into a side code path that is inside an if UNLIKELY { if UNLIKELY { ... } } block, the third nested G_UNLIKELY could maybe be dropped to make the code more readable :)
Okay. I can remove the new and third UNLIKELY from there if everyone is okay with it. Will update a patch soon.
Created attachment 291424 [details] [review] hlsdemux: If typefind fails due to short buffer (<2MB), save buffer and try again Tested on a private URL and it seems to work fine.
I have this patch on my local tree already on top of the ported hlsdemux (to adaptivedemux baseclass) and should push it together with the port soon.
commit 68e77265a00b131fa45aba45b4c1b8e4ceff3ac7 Author: Vijay Jayaraman <Vijay.Jayaraman@echostar.com> Date: Mon Nov 24 12:18:36 2014 -0700 hlsdemux: typefind might fail if first buffer is too short If typefind fails, check to see if the buffer is too short for typefind. If this is the case, prepend the decrypted buffer to the pending buffer and try again the next time around. https://bugzilla.gnome.org/show_bug.cgi?id=740458