GNOME Bugzilla – Bug 749649
multi*: Could handle properly ES KEYFRAME combined with headers
Last modified: 2018-11-03 11:37:35 UTC
Please take a look at the file here: http://195.250.34.59/temp/734124.gdp and use a pipeline like this: filesrc ! gdpdepay ! h264parse ! video/x-h264,stream-format=avc,alignment=au ! fakesink silent=false you will see that all keyframes have header flag too fakesink0:sink) (338607 bytes, dts: 0:00:15.118625403, pts: 0:00:15.152625403, duration: 0:00:00.033333333, offset: 7302216, offset_end: -1, flags: 00000400 header ) 0x7f47f008cb20 in h264parse, here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/videoparsers/gsth264parse.c#n1878 both h264parse->keyframe and h264parse->header are true, since there is no explicit flag for a keyframe how is supposed an app to find them? I based my code on the one from multihandlesink http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp/gstmultihandlesink.c#n1140 that will not recognize keyframes in this case
the header flag for the keyframes in that file are setted here: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/videoparsers/gsth264parse.c#n617
I'm not sure I understand the question or bug you are reporting. Would it be possible to clarify. If it's a bug, please explain the problem clearly. If it's a question, the mailing list might have bee a better place to start with.
seems a bug since several code assume that flag header is not set on keyframes, for example the one in multihandlesink. In my app I need to find keyframes and I use the same check as in multihandlesink and it fails
should be easy to remove the header flag in h264parse for keyframes, I can provide a patch if this is the correct approach
Created attachment 303692 [details] [review] proposed fix
Review of attachment 303692 [details] [review]: Having both a header and keyframe is completly valid, both flags should be set.
You can have a buffer that contains just a header (e.g. codec_data), you could have a buffer with just a keyframe (no header) and have a keyframe with both. I don't see anything wrong in what h264parse is doing.
I re-read the multihandlesink.c code, and what it is doing seem safe. It's not tracking headers, so the safe thing to do is to split on headers only. I think it's also fine to expect that if the buffer is just a header, the following frame will be a keyframe (not a delta unit).
Oops, actually I'm reading it wrong, a ! can be missed. > } else if (!GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_HEADER)) { > return TRUE; > } That will miss keyframe combined with headers.
(In reply to Nicolas Dufresne (stormer) from comment #9) > Oops, actually I'm reading it wrong, a ! can be missed. > > > } else if (!GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_HEADER)) { > > return TRUE; > > } > > That will miss keyframe combined with headers. yes this is the problem I have in my app too. A keyframe flag could solve the problem but this does not exist. I'm not an h264 expert however if you try the provided pipeline you'll see that pratically all buffers have header flag
What I realize, is that this particular code, including multifilesink was exclusively tested for container format, and not for ES. I still think H264 behavior is ok. The solution in multifilesink is superior to multihandlesink, though also bugged on that case. Imho, the solution is to add support for header that are also keyframes.
So what you suggest? A new flag? For now I modified my code moving the bug there: 1) if the buffer passed through a muxer I don't consider a keyframe a buffer with header flag and no delta-unit set 2) in the remaining cases if no delta-unit is set I consider the buffer a keyframe this is obviously an hack
For the multihandlesink and multifilesink, my idea was simply to add support for a buffer that is a keyframe with headers (that means HEADER is set). Other possibility is to have a keyframe without headers (nothing set), or a header without a keyframe (header and delta-unit set, even though this is a stretch), and a delta-unit. But this is all low priority for me, since I mostly care about container formats.
no problem, I don't use multi* and I'll solve in some way inside my app with an out of tree gstreamer patch or an hack in my code, thanks anyway
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/188.