GNOME Bugzilla – Bug 711627
mpegvparse: Incorrect repositioning of start code location when input buffer is empty
Last modified: 2013-11-26 11:45:54 UTC
Created attachment 259209 [details] [review] Proposed fix I have a test stream (MPEG PS) in which pictures within are GOP are terminated by a new picture start code and picture_header. During processing of this stream, a particular input buffer to mpegvparse ends with the picture start code meant to terminate the previous frame. However, there is code in mpegvparse that looks deeper into the picture header (into the temporal sequence number) to determine if the next picture is actually the bottom field of an interlaced frame. In my case, these bytes are not available in the current buffer, so we must iterate through the chain call again to get more data. When this happens, mpegvparse repositions the start_code offset back to before the previous start code so that processing can be done again: gstmpegvideoparse.c:667 GST_LOG_OBJECT (mpvparse, "need more data"); /* resume scan where we left it */ mpvparse->last_sc = size - 3; /* request best next available */ off = G_MAXUINT; goto exit; In my case, changing "size - 3" to "size - 4" fixes the problem. I think this is because the start_code is always 4 bytes, and "size" (at this point in the code) always refers to the location in the input buffer that is just after the last processed start_code. Patch included. I can provide the content file that exhibits this bug, but I have to do it through private channels (preferably Dropbox or Google Drive).
commit 7c23a6f23c185330d74ae4ccce79c3641a2fabac Author: Greg Rutz <greg@gsr-tek.com> Date: Thu Nov 7 10:57:22 2013 -0700 mpegvparse: Fix last start code position when input buffer is empty When the input buffer is empty and we need more data to determine whether or not to terminate the previous frame, the last start code location needs to be set to 4 bytes before the the current position (size of start_code is 32-bits) https://bugzilla.gnome.org/show_bug.cgi?id=711627
This was last changed from -4 to -3 btw, in: commit c5664dcda79ce17f29a583ca56af1640762b75a2 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue May 22 21:00:31 2012 +0200 mpegvideoparse: avoid scanning for start codes twice ... since a previous terminating start code serves as subsequent start code.
This change is the one that is causing the check of the TSN (and it was committed after the change you mention above). I think this change requires the reverting of Mark's change, since we need to re-parse the start_code to trigger the continued parsing of the TSN from the picture header, which will result in the decision about whether this start code will terminate the previous frame. So, I think my change is still valid. commit e5ebd7d846a296f6018cf2af32fd229a4a05f424 Author: Matej Knopp <matej.knopp@gmail.com> Date: Tue Jul 30 15:17:23 2013 +0200 mpegvideoparse: support field encoding for interlaced video https://bugzilla.gnome.org/show_bug.cgi?id=705144 diff --git a/gst/videoparsers/gstmpegvideoparse.c b/gst/videoparsers/gstmpegvideoparse.c index 073a330..b5a6d01 100644 --- a/gst/videoparsers/gstmpegvideoparse.c +++ b/gst/videoparsers/gstmpegvideoparse.c @@ -540,6 +540,21 @@ gst_mpegv_parse_process_sc (GstMpegvParse * mpvparse, else GST_LOG_OBJECT (mpvparse, "Couldn't parse picture at offset %d", mpvparse->pic_offset); + + /* if terminating packet is a picture, we need to check if it has same TSN as the pict + terminated. If it does, we need to keep those together, as these packets are two fi + frame */ + if (packet->type == GST_MPEG_VIDEO_PACKET_PICTURE) { + if (info->size - off < 2) { /* we need at least two bytes to read the TSN */ + ret = FALSE; + } else { + /* TSN is stored in first 10 bits */ + int tsn = info->data[off] << 2 | (info->data[off + 1] & 0xC0) >> 6; + + if (tsn == mpvparse->pichdr.tsn) /* prevent termination if TSN is same */ + ret = FALSE; + } + } } return ret;
I see, let's revert it then until we have a proper fix?
Created attachment 259569 [details] [review] mpegvideoparse: look beyond start code before leaping This patch should take some more cases into account and arrange for more data when needed to make a proper decision.
commit 830a4aa7f4788f3b5a874ed68a80ceae66473c47 Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Mon Nov 11 16:15:00 2013 +0100 mpegvideoparse: look beyond start code before leaping In case more data than a start code alone is needed to decide whether it ends a frame, arrange for more input data and decide when available. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=711627