[FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: support XAVC long gop

James Almer jamrial at gmail.com
Thu Apr 11 00:07:11 EEST 2019

On 4/10/2019 2:22 AM, Hendrik Leppkes wrote:
> On Wed, Apr 10, 2019 at 3:28 AM James Almer <jamrial at gmail.com> wrote:
>>>> Two thirds of SPS is not hard to implement, so i really don't understand
>>>> why you're so adamantly against it.
>>> I’m adamant about re-using code between libavcodec and libavformat.
>>> Re-using code is _good_
>> So lets do what i suggested in a previous email if re-implementing sps
>> in libavformat is not ok in your opinion: Add an
>> avpriv_h264_decode_seq_parameter_set() function that internally calls
>> ff_h264_decode_seq_parameter_set(), including proper AVBufferRef
>> cleaning at the end with a call to ff_h264_ps_uninit(), and that either
>> returns the six values using pointers parameters, or takes a pointer to
>> a new, small struct as parameter which will be allocated at runtime
>> (thus avoiding storing it on stack within libavformat, and tying to the
>> ABI) which contains at least those six values in question.
>> The reason i suggest a new small struct is to avoid using H264ParamSets
>> for this, which would imply direct access to ps->sps fields within
>> libavformat, and thus locking everything in its current offset.
>> See how avpriv_ac3_parse_header() and av_ac3_parse_header() wrap
>> ff_ac3_parse_header() and each do either of the two options above.
> As mentioned in another thread on this topic already, I agree with
> this. If you insist on re-use so strongly, then lets not cement more
> internal structs into the ABI, especially some that we worked on to
> get out of the ABI recently in the first place, and define a clean
> wrapper API that takes simple byte pointers as input and gives you a
> simplified public struct back.

I could agree to use H264ParamSets if as part of this set every single
SPS value defined in the latest version of the spec is added to "struct
SPS" and parsed by ff_h264_decode_seq_parameter_set() instead of being
discarded or stored exclusively in the AVCodecContext as it's the case
right now. The existing ones should also be double checked, seeing how i
just now found one with the wrong size (and sent a patch to fix it).
That way there will be nothing to expand in the future, and locking
fields to their current offset will not be an issue.
Ideally, the same would be done to "struct PPS", but as long as ps->pps
is not accessed from libavformat, it should be ok.

This of course will still need H264ParamSets to be allocated by the new
function to not let libavformat store it in stack, as its size must not
be part of the ABI, only the field's offsets. It would also require
ff_h264_ps_uninit to be made avpriv as it will need to be called from
outside the sps wrapper. So it's considerably a lot more work than just
using a new simple struct.

In all cases, be it a new simple struct or H264ParamSets, what must
absolutely not be used as a parameter is GetBitContext. Use instead a
data and size parameters, and allocate a GetBitContext context within
the wrapper.

> - Hendrik

More information about the ffmpeg-devel mailing list