GNOME Bugzilla – Bug 644233
Set the H.264 level from the caps for encoding
Last modified: 2011-07-13 18:17:08 UTC
Created attachment 182856 [details] [review] good: rtph264pay: Implement getcaps Here is a series of patches to negotiate the H.264 level and profile using the caps.
Created attachment 182857 [details] [review] base: codec-utils: Add method to convert H.264 text level in a level_idc Required by all the other patches
Created attachment 182858 [details] [review] ugly: x264: Use profile and level from caps
Created attachment 183025 [details] [review] ugly: x264: Use profile and level from caps Updated patch puts high10, high before main in the static caps, so if the downstream element doesnt have a default, it doesn't override the property..
I'm totally up for having this. A couple of questions: * Is profile taken into account ? It would be nice so one doesn't have to touch any encoder setting but only specify profile/level in caps (Thinking about encoding profiles). * will x264enc properly limit the accepted width/height/framerate/? according to the available downstream caps' level and profile ?
(In reply to comment #4) > * Is profile taken into account ? It would be nice so one doesn't have to touch > any encoder setting but only specify profile/level in caps (Thinking about > encoding profiles). Right now it does the "minimum" of the profile in the property and the caps. The property defaults to main. Maybe it should default to High or even None. I didn't change that as it would be an API change.. > * will x264enc properly limit the accepted width/height/framerate/? according > to the available downstream caps' level and profile ? It limits the size according to the level in setcaps and reject any incoming stream that does not meet these limits. BUT, I haven't found a good way to express limits in macroblocks/frame and macroblocks/second in video/x-raw-yuv caps, so right now if you try to send something above the level, it will just error out with not-negotiated.
Naive comment, I know, but.. shouldn't the h264 parser be modified to set level and profile in the caps as well?
(In reply to comment #6) > Naive comment, I know, but.. shouldn't the h264 parser be modified to set level > and profile in the caps as well? It could indeed, and that would be a nice addition, but it's orthogonal to the problem this bug covers.
Before I forget, we also need a "clear" listing of accepted profile/level values for h264 caps. And their type.
The only "accepted" list I could find is in codec-utils.c in pbutils.. They're all strings.
*** Bug 651318 has been marked as a duplicate of this bug. ***
What's missing now to get this committed? IMHO you should mark the property in x264enc as deprecated and only take the property value if the downstream caps don't specify a profile or level.
Downstream will always have something since I added it to the template caps. That's why I use the property as a ceiling.
Created attachment 188986 [details] [review] x264: Use profile and level from caps Updated the patch to only set the profile from the caps if it wsa in the peer caps and ignore the property in thatr case
Created attachment 188990 [details] [review] x264: Use profile and level from caps Arg, had forgotten the high profile from the if/elseif/elseif for some reason
Could we please add some minimal unit tests for this? a) that it does the right thing with profile in downstream caps b) that it does the right thing with no profile in downstream caps c) that it still respects the property
I'm not in favor of deprecating the property. Just use the profile that satisfies both the property and caps constraints. Also, one of the profiles should be "unconstrained".
David, why are you against deprecating the property? I mean surely for 0.11 we don't want both a property and caps?
The is already profile=0 which is no profile.. but it's not the default, and we can't change the default in 0.10, hence the ugly kludge.
Created attachment 189593 [details] [review] x264: Use profile and level from caps Enforces the profile and level from the downstream caps, also sets them on the fixated caps Actually, x264 doesn't do baseline, it only does constrained-baseline. So make that explicit in the caps
Created attachment 189594 [details] [review] tests: Test x264enc profiles from the caps Just for you Christian
Review of attachment 189593 [details] [review]: ::: ext/x264/gstx264enc.c @@ +1556,3 @@ + + gst_caps_unref (peer_caps); + } shouldn't we have: else { allowed_caps = peer_caps; } also, a few more comments on why the above is necessary would be good.
Created attachment 189865 [details] [review] x264enc: Select stream-format based on caps Makes x264 select its stream-format based on what's available on caps, the user selected option will be chosen as a fallback when both options are available.
Olivier any chance you could look at this sometime soon?
@Thiago: Allowed-caps is NULL if peer-caps is NULL I think. And we never fetch peer_caps directly. Comments on why what is necessary ? What does _USER mean? To use the property ? If so, wouldn't it been clearer to have it _FROM_PROPERTY
Review of attachment 189865 [details] [review]: Can we get this committed now? I think Thiagos' patch is the correct thing to do but the _USER should really be renamed to _FROM_PROPERTY (it uses the property values if the caps would allow both stream formats). And there's a small bug in his patch ::: ext/x264/gstx264enc.c @@ +1928,3 @@ nal_size = x264_nal_encode (encoder->buffer + i_size + 4, &i_data, 0, &nal[i]); + if (encoder->current_byte_stream) This is not what you meant, it will be true for _AVC and _BYTESTREAM and not for _USER. Better add a real comparison with _BYTESTREAM here for correctness and clarity and also add an assertion that the value is never _USER at this point.
Fixed Thiago's patch and committed the whole thing: BASE: commit 5f1bfc7e13bbf3329429853fffd90b3b7c96cc1e Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Mon Mar 7 17:55:48 2011 -0500 codec-utils: Add method to convert H.264 text level in a level_idc GOOD: commit 57a832cbb15cb6bdcd7b90949045ac3b2fcf6c0f Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Fri Mar 4 15:41:22 2011 -0500 rtph264pay: Implement getcaps Convert profile-level-id from RTP caps into video/x-h264 style caps (with profile and level) UGLY: commit 48f899257db49b5289f754cbd92fce246854dd0f Author: Brian Gitonga Marete <marete@toshnix.com> Date: Sat Jun 25 06:29:50 2011 +0300 x264enc: fix subme property annotation - subme maximum is 10, not 6. Although the element accepts subme values > 6, the annotation which is visible through gst-inspect (for example) erroneously indicates 6 as the maximum. Fix this by indicating 10 (which is the x264 max) as the maximum. https://bugzilla.gnome.org/show_bug.cgi?id=653473 commit e27dda7c62574d130a2e02edf3825062f3033fcd Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Mon Mar 7 17:58:34 2011 -0500 x264: Use profile and level from caps Enforces the profile and level from the downstream caps, also sets them on the fixated caps https://bugzilla.gnome.org/show_bug.cgi?id=644233 commit e595cdc3112680b3b66c6676f12ee7bc60bd11c7 Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Jun 9 20:20:27 2011 -0400 tests: Test x264enc profiles from the caps https://bugzilla.gnome.org/show_bug.cgi?id=644233 commit ac47d20faeba32339e8c7b7d45facb287f54abd2 Author: Olivier Crête <olivier.crete@collabora.com> Date: Mon Jul 4 18:03:49 2011 -0400 x264: Allow renegotiation but prefer current caps commit 7aafba6f825d7704dddfb58db180dd02e5bea479 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Mon Jun 13 23:24:27 2011 -0300 x264enc: Select stream-format based on caps Makes x264 select its stream-format based on what's available on caps, the user selected option will be chosen as a fallback when both options are available. https://bugzilla.gnome.org/show_bug.cgi?id=644233