GNOME Bugzilla – Bug 609658
[rtph264depay] doesn't mark output frames as keyframes correctly
Last modified: 2010-04-30 14:32:58 UTC
Doesn't mark output frames as keyframes correctly.
Created attachment 153617 [details] [review] Add interframe detection Probably this patch doesn't cover all cases.
Created attachment 154192 [details] [review] Patch that probably adds all cases for keyframe detection
Review of attachment 153617 [details] [review]: Substituted by a new one.
Created attachment 155292 [details] [review] Add keyframe and delta frame recognition There was a bug in the last patch.
Created attachment 158802 [details] [review] DELTA_UNIT marking of output buffers Provided patches missed out on marking of merged output buffers, i.e. when operating in access-unit output mode, which is a case where delta/key makes most sense. Attached patch takes care of that case as well, and modifies/refactors the other cases, and should as such cover the intended.
Review of attachment 158802 [details] [review]: Is this something we should get into the release as well? ::: gst/rtp/gstrtph264depay.c @@ +418,3 @@ + return FALSE; + + nal_unit_type = (GST_BUFFER_DATA (nal))[4] & 0x1f; Doesn't this mean we should check for GST_BUFFER_SIZE (nal) < 5 above? @@ +422,3 @@ + /* non-IDR VCL layer NAL considered DELTA */ + if (nal_unit_type >= 1 && nal_unit_type <= 4) { + GST_BUFFER_FLAG_SET (nal, GST_BUFFER_FLAG_DELTA_UNIT); Can we assume the buffer metadata is always writable? (Do we know the return value of _take_buffer() is always metadata-writable?)
(In reply to comment #6) > Review of attachment 158802 [details] [review]: > > Is this something we should get into the release as well? > I would not mind, probably does not hurt (unless some decoders or so turn out to be very picky w.r.t. flags). > ::: gst/rtp/gstrtph264depay.c > @@ +418,3 @@ > + return FALSE; > + > + nal_unit_type = (GST_BUFFER_DATA (nal))[4] & 0x1f; > > Doesn't this mean we should check for GST_BUFFER_SIZE (nal) < 5 above? Right. > > @@ +422,3 @@ > + /* non-IDR VCL layer NAL considered DELTA */ > + if (nal_unit_type >= 1 && nal_unit_type <= 4) { > + GST_BUFFER_FLAG_SET (nal, GST_BUFFER_FLAG_DELTA_UNIT); > > Can we assume the buffer metadata is always writable? (Do we know the return > value of _take_buffer() is always metadata-writable?) Probably not in general, but one would hope/expect so in this case, since we are taking everything that is in the adapter (and have put _new_and_alloc buffers into it), but a gst_buffer_make_metadata_writable can be added for good measure.
Created attachment 158882 [details] [review] DELTA_UNIT marking of output buffers Revised version with a few tweaks and comments incorporated.
I really don't know about the inners of the rtp depayloader. But i think it is easier to test for delta_unit and mark just before the "return" when the h264 frame is complete. Even in the case they are received fragmented, testing and marking just before the "return" will work, i think.
Created attachment 159146 [details] [review] DELTA_UNIT marking of output buffers Yet a few other tweaks in the merging case. (Btw, as mentioned above, this all pretty much marks the buffer just before return in the non-merge case, and uses appropriate caution in the merge case.)
commit 6bf7f5cfd3d7bf5e9bd923bad948fa45ca795d72 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Apr 15 12:21:56 2010 +0200 rtph264depay: DELTA_UNIT marking of output buffers ... which evidently makes (most) sense if output buffers are actually frames. Partially based on a patch by Miguel Angel Cabrera <mad_aluche at hotmail.com> Fixes #609658.