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

[Baptiste Coudurier]

> On Apr 9, 2019, at 6:27 PM, James Almer <jamrial at gmail.com> wrote:
> 
> On 4/9/2019 9:40 PM, Baptiste Coudurier wrote:
>> 
>>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial at gmail.com> wrote:
>>> 
>>> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
>>>> 
>>>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial at gmail.com> wrote:
>>>>> 
>>>>> On 4/9/2019 8:22 PM, Baptiste Coudurier wrote:
>>>>>> Hi James,
>>>>>> 
>>>>>>> On Apr 9, 2019, at 4:09 PM, James Almer <jamrial at gmail.com> wrote:
>>>>>>> 
>>>>>>>> […]
>>>>>>> 
>>>>>>> Ok so, the only fields you need from the sps are constraint_set_flags,
>>>>>>> frame_mbs_only_flag, bit_depth_luma, profile_idc, level_idc and sar,
>>>>>>> meaning you don't even need to parse the sps until the very end (sar is
>>>>>>> the furthest value, and right at the beginning of vui_parameters).
>>>>>>> I'd very much prefer if we can avoid making
>>>>>>> ff_h264_decode_seq_parameter_set() avpriv and with it H264ParamSets part
>>>>>>> of the ABI, so i think it's worth copying the required bitstream parsing
>>>>>>> code at least until the beginning of vui_parameters. It was done for
>>>>>>> hevc and av1, so it can be done for h264 as well.
>>>>>> 
>>>>>> Since vui parsing is needed, that would be the entire "ff_h264_decode_seq_parameter_set()” function.
>>>>> 
>>>>> If you only need up to sar, then yo don't need to parse the entire VUI.
>>>> 
>>>> It still requires copying the entire ff_h264_decode_seq_parameter_set(), even though the sub functions are not needed. 
>>>> 
>>>>>> 
>>>>>> I disagree with the copying. I also disagree with the copying for AVC1 and HEVC btw.
>>>>> 
>>>>> Using avpriv opens a whole can of worms that will be unpredictable in
>>>>> the future. Just look at recent commits like
>>>>> f4ea930a119298c6110ee4e3d24219a66e27e230 that started storing previously
>>>>> skipped values. If hevc_pc was made avpriv for isombff/matroska muxing
>>>>> purposes and its structs part of the ABI, this wouldn't have been
>>>>> possible until a major bump.
>>>> 
>>>> When did the sharing policy between avformat and avcodec (in the sense avformat using avcodec) change ?
>>>> It seems perfectly natural to me to have libavformat require the libavcodec version that it was compiled with,
>>>> and check it at runtime.
>>> 
>>> More than one ffmpeg release will ship with libavcodec/libavformat
>>> sharing the same soname. Backwards compatibility is expected, hence the
>>> split between major, minor and micro, as you're meant to be able to drop
>>> a newer libavcodec library, say for example ffmpeg 4.1's
>>> libavcodec.58.so, and it should work with an existing libavformat.so.58
>>> that was linked against ffmpeg 4.0's libavcodec.58.so.
>> 
>> Sorry to repeat the question but when did that change ?
> 
> I don't know if it ever changed to being with. It's been like this for
> as long as i remember, and i don't recall inter-library version checks
> that would take minor and micro into consideration (which would be
> needed to truly tie a given libavformat library with the exact
> libavcodec it was linked to) whereas ABI backwards compatibility
> considerations for inter-library stuff can be seen all across the
> codebase and git history.

It definitely changed, probably around the introduction of “avpriv” prefix.

>> Which practical use case are we covering here ?
> 
> I don't know, i'm not a library user. But i know the main use case for
> the longest time was having ffmpeg work as a drop in replacement for
> libav back when the latter was shipped in debian and other ditros.
> 
>> You were never supposed to update libavcodec without updating libavformat
>> If you use libavformat in the first place.
>> Your problems are completely gone.
> 
> As i said, as far as i remember, you've been meant to do that just fine.
> Hence so much work about reducing the amount of exposed internal API,
> and plenty of preprocessor checks meant to remove unneeded avpriv
> symbols only after a major bump.

Well, AFAIR it was not the case, and IMHO it does not really make sense to
update libavcodec without libavformat.
I think all that work could have been avoided by introducing an explicit check
inside libavformat.
It would also improve code-reuse and peace of mind, not worrying about 
the ABI within the project.

I think a distinction should be made between breaking the ABI for the user
and the ABI used within the project.  

>> 
>>> 
>>>> 
>>>>> 
>>>>> sps parsing can and should be done in libavformat for this. Otherwise
>>>>> you'll be constraining future development in other areas. So please,
>>>>> take libavcodec/cbs_h264* and libavformat/av1.c as basis and write a
>>>>> simple sps parser that takes the raw bitstream values and does nothing
>>>>> else. You only need six values.
>>>> 
>>>> I disagree with the copying and we will need more opinions on this to settle
>>>> the argument, maybe a vote.
>>> 
>>> It's the cleanest way and the one used in all cases previous to this
>>> one.
>> 
>> We disagree here, there are at least 2 cleaner ways: not using AVCodecContext at all
>> in sps parsing or forcing libavcodec version.
> 
> You'd still be making GetBitContext part of the ABI with the former option.

Well IMHO GetBitContext is SO useful that it should probably be moved to libavutil and made public.

> 
>> 
>>> There has been work during the last bump to remove internal structs
>>> from the ABI precisely because they constrained development. Among them
>>> was GetBitsContext, which you're now trying to reintroduce and thus
>>> invalidate all that previous cleaning work, and only to get six values
>>> for a single muxer.
>> 
>> Adding fields are the end of structs does not break ABI, it’s been done for decades in FFmpeg.
> 
> Only for structs which size is not part of the ABI, and thus can't be
> stored on stack. That means those allocated with a custom alloc
> function, which is for example the case of AVCodecContext and AVFrame,
> but not AVPacket.

Please correct me if I’m wrong: all structures with publicly exported types with
determined size are part of the ABI.
If the user chooses to store the struct on the stack or to alloc manually,
he voluntarily chooses to be exposed to ABI issues and that case is out of scope IMHO.

To be ABI safe, user needs to use allocation functions provided by the library, and structs fields offsets 
cannot be changed.

> 
>> Also looking at h264_ps.h:
>> 
>> […]
>>    int residual_color_transform_flag;    ///< residual_colour_transform_flag
>>    int constraint_set_flags;             ///< constraint_set[0-3]_flag
>>    uint8_t data[4096];
>>    size_t data_size;
>> 
>> Why are we wasting 4k here ? I’m trying to trace git logs but it seems to be originating
>> from a merge commit.
> 
> I'm guessing it was done to avoid having data pointers within a struct
> stored in an AVBufferRef, as those are meant to have flat data arrays.

I dug and it’s used to store original bitstream data for videotoolbox usage…
I’m not sure I understand here why an allocated buffer wasn’t used or another more
elegant solution.

> 
>> 
>>> 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.

I just copied the code after all, you win :) I had to change it, I’m not even sure that decode scaling matrix
is 100% correct. I really feel ashamed, throwing out decades of testing of the original code...

— 
Baptiste