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

[Baptiste Coudurier]

> 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 ?
Which practical use case are we covering here ?
You were never supposed to update libavcodec without updating libavformat
If you use libavformat in the first place.
Your problems are completely gone.

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

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

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

— 
Baptiste