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

Submitted by Baptiste Coudurier on April 9, 2019, 10:14 p.m.

Details

Message ID 20190409221403.41981-2-baptiste.coudurier@gmail.com
State New
Headers show

Commit Message

Baptiste Coudurier April 9, 2019, 10:14 p.m.
---
 libavformat/Makefile |   2 +-
 libavformat/avc.c    |  32 +++++++
 libavformat/avc.h    |   2 +
 libavformat/hevc.c   |  36 +-------
 libavformat/mxf.h    |   1 +
 libavformat/mxfenc.c | 201 +++++++++++++++++++++++++++++++++----------
 6 files changed, 193 insertions(+), 81 deletions(-)

Comments

Hendrik Leppkes April 9, 2019, 11:05 p.m.
On Wed, Apr 10, 2019 at 12:21 AM Baptiste Coudurier
<baptiste.coudurier@gmail.com> wrote:
> +                return 0;
> +            }
> +            init_get_bits(&gb, tmp, tmp_size*8);
> +            ret = avpriv_h264_decode_seq_parameter_set(&gb, (AVCodecContext*)s, &ps, 0);

The AVCodecContext cast looks like a recipe for disaster. The function
is internal to the H264 decoder, so if it at some point decides to
actually access the AVCodecContext it receives, or even worse, the
priv_data element in it, expecting a H264Context, then everything goes
to hell.
Just another sign why exporting functions internal from a decoder is
just an overall bad idea...

- Hendrik
James Almer April 9, 2019, 11:09 p.m.
On 4/9/2019 7:14 PM, Baptiste Coudurier wrote:
> ---
>  libavformat/Makefile |   2 +-
>  libavformat/avc.c    |  32 +++++++
>  libavformat/avc.h    |   2 +
>  libavformat/hevc.c   |  36 +-------
>  libavformat/mxf.h    |   1 +
>  libavformat/mxfenc.c | 201 +++++++++++++++++++++++++++++++++----------
>  6 files changed, 193 insertions(+), 81 deletions(-)
> 
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index c010fc83f9..f8539527bc 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -338,7 +338,7 @@ OBJS-$(CONFIG_MUSX_DEMUXER)              += musx.o
>  OBJS-$(CONFIG_MV_DEMUXER)                += mvdec.o
>  OBJS-$(CONFIG_MVI_DEMUXER)               += mvi.o
>  OBJS-$(CONFIG_MXF_DEMUXER)               += mxfdec.o mxf.o
> -OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o audiointerleave.o
> +OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o audiointerleave.o avc.o
>  OBJS-$(CONFIG_MXG_DEMUXER)               += mxg.o
>  OBJS-$(CONFIG_NC_DEMUXER)                += ncdec.o
>  OBJS-$(CONFIG_NISTSPHERE_DEMUXER)        += nistspheredec.o pcm.o
> diff --git a/libavformat/avc.c b/libavformat/avc.c
> index ec50033a04..6c97ae04ab 100644
> --- a/libavformat/avc.c
> +++ b/libavformat/avc.c
> @@ -241,3 +241,35 @@ const uint8_t *ff_avc_mp4_find_startcode(const uint8_t *start,
>  
>      return start + res;
>  }
> +
> +uint8_t *ff_nal_unit_extract_rbsp(const uint8_t *src, uint32_t src_len,
> +                                  uint32_t *dst_len, int header_len)

Nit: Might be a good chance to make src_len and dst_len size_t.

> +{
> +    uint8_t *dst;
> +    uint32_t i, len;
> +
> +    dst = av_malloc(src_len + AV_INPUT_BUFFER_PADDING_SIZE);
> +    if (!dst)
> +        return NULL;
> +
> +    /* NAL unit header */
> +    i = len = 0;
> +    while (i < header_len && i < src_len)
> +        dst[len++] = src[i++];
> +
> +    while (i + 2 < src_len)
> +        if (!src[i] && !src[i + 1] && src[i + 2] == 3) {
> +            dst[len++] = src[i++];
> +            dst[len++] = src[i++];
> +            i++; // remove emulation_prevention_three_byte
> +        } else
> +            dst[len++] = src[i++];
> +
> +    while (i < src_len)
> +        dst[len++] = src[i++];
> +
> +    memset(dst + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +
> +    *dst_len = len;
> +    return dst;
> +}

[...]

> @@ -2168,37 +2227,81 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
>  {
>      MXFContext *mxf = s->priv_data;
>      MXFStreamContext *sc = st->priv_data;
> -    AVCodecParameters *par = st->codecpar;
> -    static const int mxf_h264_num_codec_uls = sizeof(mxf_h264_codec_uls) / sizeof(mxf_h264_codec_uls[0]);
> +    H264ParamSets ps = { { 0 } };
> +    GetBitContext gb;
>      const uint8_t *buf = pkt->data;
>      const uint8_t *buf_end = pkt->data + pkt->size;
> +    const uint8_t *tmp, *nal_end;
>      uint32_t state = -1;
> -    int long_gop = 0; // assume intra when there is no SPS header
>      int extra_size = 512; // support AVC Intra files without SPS/PPS header
> -    int i, frame_size;
> -    uint8_t uid_found;
> -
> -    if (pkt->size > extra_size)
> -        buf_end -= pkt->size - extra_size; // no need to parse beyond SPS/PPS header
> +    int i, j, tmp_size, frame_size, ret, slice_type, intra_only = 0;
>  
>      for (;;) {
>          buf = avpriv_find_start_code(buf, buf_end, &state);
>          if (buf >= buf_end)
>              break;
> -        --buf;
> +
>          switch (state & 0x1f) {
>          case H264_NAL_SPS:
> -            par->profile = buf[1];
> -            long_gop = buf[2] & 0x10 ? 0 : 1; // constraint_set3_flag signals intra
>              e->flags |= 0x40;
> -            break;
> +
> +            if (ps.sps != NULL)
> +                break;
> +
> +            nal_end = ff_avc_find_startcode(buf, buf_end);
> +            tmp = ff_nal_unit_extract_rbsp(buf, nal_end - buf, &tmp_size, 0);
> +            if (!buf) {
> +                av_log(s, AV_LOG_ERROR, "error extracting rbsp\n");
> +                return 0;
> +            }
> +            init_get_bits(&gb, tmp, tmp_size*8);
> +            ret = avpriv_h264_decode_seq_parameter_set(&gb, (AVCodecContext*)s, &ps, 0);
> +            av_freep(&tmp);
> +            if (ret < 0)
> +                return 0;
> +            for (j = 0; j < MAX_SPS_COUNT; j++) {
> +                if (ps.sps_list[j] != NULL) {
> +                    ps.sps = (SPS*)ps.sps_list[j]->data;
> +                    break;
> +                }
> +            }
> +
> +            sc->aspect_ratio.num = st->codecpar->width * ps.sps->sar.num;
> +            sc->aspect_ratio.den = st->codecpar->height * ps.sps->sar.den;
> +            av_reduce(&sc->aspect_ratio.num, &sc->aspect_ratio.den,
> +                      sc->aspect_ratio.num, sc->aspect_ratio.den, 1024*1024);
> +            intra_only = (ps.sps->constraint_set_flags >> 3) & 1;
> +            sc->interlaced = !ps.sps->frame_mbs_only_flag;
> +            sc->component_depth = ps.sps->bit_depth_luma;
> +
> +            buf = nal_end;
> +            continue;
>          case H264_NAL_PPS:
>              if (e->flags & 0x40) { // sequence header present
>                  e->flags |= 0x80; // random access
>                  extra_size = 0;
> -                buf = buf_end;
>              }
>              break;
> +        case H264_NAL_IDR_SLICE:
> +            e->flags |= 0x04; // IDR Picture
> +            buf = buf_end;
> +            break;
> +        case H264_NAL_SLICE:
> +            init_get_bits(&gb, buf, buf_end - buf);
> +            get_ue_golomb_long(&gb); // skip first_mb_in_slice
> +            slice_type = get_ue_golomb_31(&gb);
> +            switch (slice_type % 5) {
> +            case 0:
> +                e->flags |= 0x20; // P Picture
> +                e->flags |= 0x06; // P Picture
> +                break;
> +            case 1:
> +                e->flags |= 0x30; // B Picture
> +                e->flags |= 0x03; // non-referenced B Picture
> +                break;
> +            }
> +            buf = buf_end;
> +            break;
>          default:
>              break;
>          }
> @@ -2207,27 +2310,35 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
>      if (mxf->header_written)
>          return 1;
>  
> -    sc->aspect_ratio = (AVRational){ 16, 9 }; // 16:9 is mandatory for broadcast HD
> -    sc->interlaced = par->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0;
> -
> -    uid_found = 0;
> +    if (!ps.sps)
> +        sc->interlaced = st->codecpar->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0;
> +    sc->codec_ul = NULL;
>      frame_size = pkt->size + extra_size;
> -    for (i = 0; i < mxf_h264_num_codec_uls; i++) {
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(mxf_h264_codec_uls); i++) {
>          if (frame_size == mxf_h264_codec_uls[i].frame_size && sc->interlaced == mxf_h264_codec_uls[i].interlaced) {
>              sc->codec_ul = &mxf_h264_codec_uls[i].uid;
>              sc->component_depth = 10; // AVC Intra is always 10 Bit
> +            sc->aspect_ratio = (AVRational){ 16, 9 }; // 16:9 is mandatory for broadcast HD
> +            st->codecpar->profile = mxf_h264_codec_uls[i].profile;
> +            sc->avc_intra = 1;
>              if (sc->interlaced)
>                  sc->field_dominance = 1; // top field first is mandatory for AVC Intra
> +            mxf->cbr_index = 1;
> +            sc->frame_size = frame_size;
>              return 1;
> -        } else if ((mxf_h264_codec_uls[i].profile == par->profile) &&
> -                   ((mxf_h264_codec_uls[i].long_gop < 0) ||
> -                   (mxf_h264_codec_uls[i].long_gop == long_gop))) {
> +        } else if (ps.sps && mxf_h264_codec_uls[i].frame_size == 0 &&
> +                   mxf_h264_codec_uls[i].profile == ps.sps->profile_idc &&
> +                   (mxf_h264_codec_uls[i].intra_only < 0 ||
> +                    mxf_h264_codec_uls[i].intra_only == intra_only)) {
>              sc->codec_ul = &mxf_h264_codec_uls[i].uid;
> -            uid_found = 1;
> +            st->codecpar->profile = ps.sps->profile_idc;
> +            st->codecpar->level = ps.sps->level_idc;
> +            // continue to check for avc intra
>          }
>      }
>  
> -    if (!uid_found) {
> +    if (!sc->codec_ul) {
>          av_log(s, AV_LOG_ERROR, "h264 profile not supported\n");
>          return 0;
>      }

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.

If you look a the h264 module from the CBS framework (cbs_h264) in
libavcodec, you'll find a very simple parsing implementation that
doesn't bother to decode the values, or fill tables, or allocate any
kind of buffer like the h264_ps.c does. You in fact will not need to
even store any value beyond the ones i listed above.
Writing a reusable and expansible-as-needed sps parsing function in
libavformat shouldn't be hard with the above module, and would be much
cleaner and less constraining in the long run for both libavformat and
libavcodec.
Baptiste Coudurier April 9, 2019, 11:22 p.m.
Hi James,

> On Apr 9, 2019, at 4:09 PM, James Almer <jamrial@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.

I disagree with the copying. I also disagree with the copying for AVC1 and HEVC btw.

— 
Baptiste
Baptiste Coudurier April 9, 2019, 11:25 p.m.
> On Apr 9, 2019, at 4:05 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> 
> On Wed, Apr 10, 2019 at 12:21 AM Baptiste Coudurier
> <baptiste.coudurier@gmail.com> wrote:
>> +                return 0;
>> +            }
>> +            init_get_bits(&gb, tmp, tmp_size*8);
>> +            ret = avpriv_h264_decode_seq_parameter_set(&gb, (AVCodecContext*)s, &ps, 0);
> 
> The AVCodecContext cast looks like a recipe for disaster. The function
> is internal to the H264 decoder, so if it at some point decides to
> actually access the AVCodecContext it receives, or even worse, the
> priv_data element in it, expecting a H264Context, then everything goes
> to hell.
> Just another sign why exporting functions internal from a decoder is
> just an overall bad idea…

Calm down. AVCodecContext can be allocated in any case, for AV_CODEC_ID_H264.
The reason that function needs it in the first place is small.

Re-using code is _good_

— 
Baptiste
James Almer April 9, 2019, 11:51 p.m.
On 4/9/2019 8:22 PM, Baptiste Coudurier wrote:
> Hi James,
> 
>> On Apr 9, 2019, at 4:09 PM, James Almer <jamrial@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.

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

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.

> 
> — 
> Baptiste
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Baptiste Coudurier April 9, 2019, 11:59 p.m.
> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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.

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

— 
Baptiste
James Almer April 10, 2019, 12:24 a.m.
On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
> 
>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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.

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

Two thirds of SPS is not hard to implement, so i really don't understand
why you're so adamantly against it.

> 
> — 
> Baptiste
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Baptiste Coudurier April 10, 2019, 12:40 a.m.
> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:
> 
> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
>> 
>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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
James Almer April 10, 2019, 1:27 a.m.
On 4/9/2019 9:40 PM, Baptiste Coudurier wrote:
> 
>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:
>>
>> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
>>>
>>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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.

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

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

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

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

> 
>> 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.
Hendrik Leppkes April 10, 2019, 5:22 a.m.
On Wed, Apr 10, 2019 at 3:28 AM James Almer <jamrial@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.

- Hendrik
Carl Eugen Hoyos April 10, 2019, 8:26 p.m.
2019-04-10 2:40 GMT+02:00, Baptiste Coudurier <baptiste.coudurier@gmail.com>:
>
>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:

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

While we have violated this before, we of course claim
compatibility, so this argument is not helpful, sorry.

Carl Eugen
James Almer April 10, 2019, 9:07 p.m.
On 4/10/2019 2:22 AM, Hendrik Leppkes wrote:
> On Wed, Apr 10, 2019 at 3:28 AM James Almer <jamrial@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
Baptiste Coudurier April 10, 2019, 9:26 p.m.
> On Apr 9, 2019, at 6:27 PM, James Almer <jamrial@gmail.com> wrote:
> 
> On 4/9/2019 9:40 PM, Baptiste Coudurier wrote:
>> 
>>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:
>>> 
>>> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
>>>> 
>>>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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
James Almer April 10, 2019, 9:54 p.m.
On 4/10/2019 6:26 PM, Baptiste Coudurier wrote:
>> On Apr 9, 2019, at 6:27 PM, James Almer <jamrial@gmail.com> wrote:
>>
>> On 4/9/2019 9:40 PM, Baptiste Coudurier wrote:
>>>
>>>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:
>>>>
>>>> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
>>>>>
>>>>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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.

I think that predates my arrival to the project. Someone else will have
to chime in...

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

I wouldn't be against it if it can be done right. But then there's also
libavdevice which is tied to libavformat, so ultimately you'd be locking
every single library to specific versions, and at that point, you'd be
better just merging everything into a single monolithic library called
libffmpeg.so, libav.so or whatever.

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

You can use GetBitContext from any library you want. It's a standalone
header. We're using it in a lot of muxers from libavformat.
What you can't (or shouldn't) do is making it a parameter in an exported
function, as you'd be locking the struct.

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

Indeed, and it's stated as such. Structs were we explicitly state the
size is not part of the ABI are not meant to be stored on stack. If the
user does it anyway, then they are accepting potential issues whenever a
new field is added at the end. We can't stop users from misusing the API
after all.
Structs in public headers were we don't state anything about its size
can't be changed at all unless a major bump takes place, as the user is
allowed to store them on stack.

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

Yes, that's what's done for all structs were we state the size is not
part of the ABI. Offsets of existing fields are locked, but fields can
be added at the end just fine. If the user ignores the alloc function
and does a malloc(sizeof(AVFoo)) then they are asking for trouble.

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

As i said, the struct is stored within an AVBufferRef. Those are meant
to have flat arrays and not pointers to other buffers, as we have no way
to fully control their lifetime, even with a custom free() callback.
It's why some frame side data types are so awkward, like
AV_FRAME_DATA_QP_TABLE_PROPERTIES and AV_FRAME_DATA_QP_TABLE_DATA.

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

I don't want to win, i want a solution that will not constrain further
libavcodec development to major bump periods. I don't want HWAccel
people to hate us because a new Nvidia SDK wants a field we were not
already exporting.
And considering i found a field with wrong size in the SPS struct that
went undetected for years, there's no such thing as enough testing...

In any case, the scaling matrix is implemented in a very simple way in
CBS, so assuming it's correct you're better using that as basis rather
than ff_h264_decode_seq_parameter_set().

And if others agree, you can still do what i suggested above with the
extra considerations in my latest email from an hour or so ago.

> 
> — 
> Baptiste
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer April 16, 2019, 11:03 p.m.
On Wed, Apr 10, 2019 at 06:54:53PM -0300, James Almer wrote:
> On 4/10/2019 6:26 PM, Baptiste Coudurier wrote:
> >> On Apr 9, 2019, at 6:27 PM, James Almer <jamrial@gmail.com> wrote:
> >>
> >> On 4/9/2019 9:40 PM, Baptiste Coudurier wrote:
> >>>
> >>>> On Apr 9, 2019, at 5:24 PM, James Almer <jamrial@gmail.com> wrote:
> >>>>
> >>>> On 4/9/2019 8:59 PM, Baptiste Coudurier wrote:
> >>>>>
> >>>>>> On Apr 9, 2019, at 4:51 PM, James Almer <jamrial@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@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.
> 
> I think that predates my arrival to the project. Someone else will have
> to chime in...

it possibly changed from "we dont care / we didnt think about it" in the 
distant past, iam not sure its very long ago.
But strange interdependancies on minor/micro versions that are outside
the established rules (https://semver.org/)
can cause surprises and problems to for example packagers for distros.
So i suggest to stay close to what most users of the libs / packagers do
expect and whould causes the least problems and surprise ...

Thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/Makefile b/libavformat/Makefile
index c010fc83f9..f8539527bc 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -338,7 +338,7 @@  OBJS-$(CONFIG_MUSX_DEMUXER)              += musx.o
 OBJS-$(CONFIG_MV_DEMUXER)                += mvdec.o
 OBJS-$(CONFIG_MVI_DEMUXER)               += mvi.o
 OBJS-$(CONFIG_MXF_DEMUXER)               += mxfdec.o mxf.o
-OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o audiointerleave.o
+OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o audiointerleave.o avc.o
 OBJS-$(CONFIG_MXG_DEMUXER)               += mxg.o
 OBJS-$(CONFIG_NC_DEMUXER)                += ncdec.o
 OBJS-$(CONFIG_NISTSPHERE_DEMUXER)        += nistspheredec.o pcm.o
diff --git a/libavformat/avc.c b/libavformat/avc.c
index ec50033a04..6c97ae04ab 100644
--- a/libavformat/avc.c
+++ b/libavformat/avc.c
@@ -241,3 +241,35 @@  const uint8_t *ff_avc_mp4_find_startcode(const uint8_t *start,
 
     return start + res;
 }
+
+uint8_t *ff_nal_unit_extract_rbsp(const uint8_t *src, uint32_t src_len,
+                                  uint32_t *dst_len, int header_len)
+{
+    uint8_t *dst;
+    uint32_t i, len;
+
+    dst = av_malloc(src_len + AV_INPUT_BUFFER_PADDING_SIZE);
+    if (!dst)
+        return NULL;
+
+    /* NAL unit header */
+    i = len = 0;
+    while (i < header_len && i < src_len)
+        dst[len++] = src[i++];
+
+    while (i + 2 < src_len)
+        if (!src[i] && !src[i + 1] && src[i + 2] == 3) {
+            dst[len++] = src[i++];
+            dst[len++] = src[i++];
+            i++; // remove emulation_prevention_three_byte
+        } else
+            dst[len++] = src[i++];
+
+    while (i < src_len)
+        dst[len++] = src[i++];
+
+    memset(dst + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+
+    *dst_len = len;
+    return dst;
+}
diff --git a/libavformat/avc.h b/libavformat/avc.h
index c5e80ff650..9f9b4c0175 100644
--- a/libavformat/avc.h
+++ b/libavformat/avc.h
@@ -33,5 +33,7 @@  int ff_avc_write_annexb_extradata(const uint8_t *in, uint8_t **buf, int *size);
 const uint8_t *ff_avc_mp4_find_startcode(const uint8_t *start,
                                          const uint8_t *end,
                                          int nal_length_size);
+uint8_t *ff_nal_unit_extract_rbsp(const uint8_t *src, uint32_t src_len,
+                                  uint32_t *dst_len, int header_len);
 
 #endif /* AVFORMAT_AVC_H */
diff --git a/libavformat/hevc.c b/libavformat/hevc.c
index 3628d5a028..c7c4be3441 100644
--- a/libavformat/hevc.c
+++ b/libavformat/hevc.c
@@ -643,40 +643,6 @@  static int hvcc_parse_pps(GetBitContext *gb,
     return 0;
 }
 
-static uint8_t *nal_unit_extract_rbsp(const uint8_t *src, uint32_t src_len,
-                                      uint32_t *dst_len)
-{
-    uint8_t *dst;
-    uint32_t i, len;
-
-    dst = av_malloc(src_len + AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!dst)
-        return NULL;
-
-    /* NAL unit header (2 bytes) */
-    i = len = 0;
-    while (i < 2 && i < src_len)
-        dst[len++] = src[i++];
-
-    while (i + 2 < src_len)
-        if (!src[i] && !src[i + 1] && src[i + 2] == 3) {
-            dst[len++] = src[i++];
-            dst[len++] = src[i++];
-            i++; // remove emulation_prevention_three_byte
-        } else
-            dst[len++] = src[i++];
-
-    while (i < src_len)
-        dst[len++] = src[i++];
-
-    memset(dst + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
-
-    *dst_len = len;
-    return dst;
-}
-
-
-
 static void nal_unit_parse_header(GetBitContext *gb, uint8_t *nal_type)
 {
     skip_bits1(gb); // forbidden_zero_bit
@@ -753,7 +719,7 @@  static int hvcc_add_nal_unit(uint8_t *nal_buf, uint32_t nal_size,
     uint8_t *rbsp_buf;
     uint32_t rbsp_size;
 
-    rbsp_buf = nal_unit_extract_rbsp(nal_buf, nal_size, &rbsp_size);
+    rbsp_buf = ff_nal_unit_extract_rbsp(nal_buf, nal_size, &rbsp_size, 2);
     if (!rbsp_buf) {
         ret = AVERROR(ENOMEM);
         goto end;
diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index 4394450dea..f32124f772 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -48,6 +48,7 @@  enum MXFMetadataSetType {
     EssenceGroup,
     TaggedValue,
     TapeDescriptor,
+    AVCSubDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 8c6db94865..8f565e39f8 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -49,12 +49,14 @@ 
 #include "libavcodec/bytestream.h"
 #include "libavcodec/dnxhddata.h"
 #include "libavcodec/dv_profile.h"
-#include "libavcodec/h264.h"
+#include "libavcodec/h264_ps.h"
+#include "libavcodec/golomb.h"
 #include "libavcodec/internal.h"
 #include "audiointerleave.h"
 #include "avformat.h"
 #include "avio_internal.h"
 #include "internal.h"
+#include "avc.h"
 #include "mxf.h"
 #include "config.h"
 
@@ -99,6 +101,7 @@  typedef struct MXFStreamContext {
     int max_gop;             ///< maximum gop size, used by mpeg-2 descriptor
     int b_picture_count;     ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor
     int low_delay;           ///< low delay, used in mpeg-2 descriptor
+    int avc_intra;
 } MXFStreamContext;
 
 typedef struct MXFContainerEssenceEntry {
@@ -167,6 +170,7 @@  static const struct {
 static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st);
@@ -310,7 +314,7 @@  static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x0D,0x01,0x03,0x01,0x02,0x10,0x60,0x01 },
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x15,0x01,0x05,0x00 },
       { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x00,0x00,0x00 },
-      mxf_write_mpegvideo_desc },
+      mxf_write_h264_desc },
     // S436M ANC
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x0D,0x01,0x03,0x01,0x02,0x0e,0x00,0x00 },
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x17,0x01,0x02,0x00 },
@@ -498,6 +502,12 @@  static const MXFLocalTagPair mxf_local_tag_batch[] = {
     { 0x8006, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x01,0x06,0x02,0x01,0x08,0x00,0x00}}, /* MaxGOP */
     { 0x8007, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x01,0x06,0x02,0x01,0x0A,0x00,0x00}}, /* ProfileAndLevel */
     { 0x8008, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x01,0x06,0x02,0x01,0x09,0x00,0x00}}, /* BPictureCount */
+    // Generic Descriptor
+    { 0x8100, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x04,0x06,0x10,0x00,0x00}}, /* SubDescriptors */
+    // AVC SubDescriptor
+    { 0x8200, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x06,0x01,0x0E,0x00,0x00}}, /* AVC Decoding Delay */
+    { 0x8201, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x06,0x01,0x0A,0x00,0x00}}, /* AVC Profile */
+    { 0x8202, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x06,0x01,0x0D,0x00,0x00}}, /* AVC Level */
     // Wave Audio Essence Descriptor
     { 0x3D09, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x03,0x03,0x05,0x00,0x00,0x00}}, /* Average Bytes Per Second */
     { 0x3D0A, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x05,0x04,0x02,0x03,0x02,0x01,0x00,0x00,0x00}}, /* Block Align */
@@ -1126,6 +1136,8 @@  static const UID mxf_aes3_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,
 static const UID mxf_cdci_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x28,0x00 };
 static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x42,0x00 };
 
+static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
+
 static int get_trc(UID ul, enum AVColorTransferCharacteristic trc)
 {
     switch (trc){
@@ -1317,6 +1329,13 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         avio_w8(pb, sc->field_dominance);
     }
 
+    if (st->codecpar->codec_id == AV_CODEC_ID_H264 && !sc->avc_intra) {
+        // write avc sub descriptor ref
+        mxf_write_local_tag(pb, 8 + 16, 0x8100);
+        mxf_write_refs_count(pb, 1);
+        mxf_write_uuid(pb, AVCSubDescriptor, 0);
+    }
+
     return pos;
 }
 
@@ -1329,10 +1348,50 @@  static void mxf_update_klv_size(AVIOContext *pb, int64_t pos)
     avio_seek(pb, cur_pos, SEEK_SET);
 }
 
+static void mxf_write_avc_subdesc(AVFormatContext *s, AVStream *st)
+{
+    AVIOContext *pb = s->pb;
+    int64_t pos;
+
+    avio_write(pb, mxf_avc_subdescriptor_key, 16);
+    klv_encode_ber4_length(pb, 0);
+    pos = avio_tell(pb);
+
+    mxf_write_local_tag(pb, 16, 0x3C0A);
+    mxf_write_uuid(pb, AVCSubDescriptor, 0);
+
+    mxf_write_local_tag(pb, 1, 0x8200);
+    avio_w8(pb, 0xFF); // AVC Decoding Delay, unknown
+
+    mxf_write_local_tag(pb, 1, 0x8201);
+    avio_w8(pb, st->codecpar->profile); // AVC Profile
+
+    mxf_write_local_tag(pb, 1, 0x8202);
+    avio_w8(pb, st->codecpar->level); // AVC Level
+
+    mxf_update_klv_size(s->pb, pos);
+}
+
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st)
 {
     int64_t pos = mxf_write_cdci_common(s, st, mxf_cdci_descriptor_key);
     mxf_update_klv_size(s->pb, pos);
+
+    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
+        mxf_write_avc_subdesc(s, st);
+    }
+}
+
+static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st)
+{
+    MXFStreamContext *sc = st->priv_data;
+    if (sc->avc_intra) {
+        mxf_write_mpegvideo_desc(s, st);
+    } else {
+        int64_t pos = mxf_write_cdci_common(s, st, mxf_cdci_descriptor_key);
+        mxf_update_klv_size(s->pb, pos);
+        mxf_write_avc_subdesc(s, st);
+    }
 }
 
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st)
@@ -2136,30 +2195,30 @@  static const struct {
     int frame_size;
     int profile;
     uint8_t interlaced;
-    int long_gop; // 1 or 0 when there are separate UIDs for Long GOP and Intra, -1 when Intra/LGOP detection can be ignored
+    int intra_only; // 1 or 0 when there are separate UIDs for Long GOP and Intra, -1 when Intra/LGOP detection can be ignored
 } mxf_h264_codec_uls[] = {
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x11,0x01 },      0,  66, 0, -1 }, // AVC Baseline, Unconstrained Coding
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x11,0x01 },      0,  66, 0, -1 }, // AVC Baseline
     {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x20,0x01 },      0,  77, 0, -1 }, // AVC Main
     {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x30,0x01 },      0,  88, 0, -1 }, // AVC Extended
     {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x40,0x01 },      0, 100, 0, -1 }, // AVC High
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x50,0x01 },      0, 110, 0,  1 }, // AVC High 10
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x60,0x01 },      0, 122, 0,  1 }, // AVC High 422
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x70,0x01 },      0, 244, 0,  1 }, // AVC High 444
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x20,0x01 },      0, 110, 0,  0 }, // AVC High 10 Intra
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x01 }, 232960,   0, 1,  0 }, // AVC Intra 50 1080i60
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x02 }, 281088,   0, 1,  0 }, // AVC Intra 50 1080i50
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x03 }, 232960,   0, 0,  0 }, // AVC Intra 50 1080p30
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x04 }, 281088,   0, 0,  0 }, // AVC Intra 50 1080p25
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x08 }, 116736,   0, 0,  0 }, // AVC Intra 50 720p60
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x09 }, 140800,   0, 0,  0 }, // AVC Intra 50 720p50
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x30,0x01 },      0, 122, 0,  0 }, // AVC High 422 Intra
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x01 }, 472576,   0, 1,  0 }, // AVC Intra 100 1080i60
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x02 }, 568832,   0, 1,  0 }, // AVC Intra 100 1080i50
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x03 }, 472576,   0, 0,  0 }, // AVC Intra 100 1080p30
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x04 }, 568832,   0, 0,  0 }, // AVC Intra 100 1080p25
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x08 }, 236544,   0, 0,  0 }, // AVC Intra 100 720p60
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x09 }, 284672,   0, 0,  0 }, // AVC Intra 100 720p50
-    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x01,0x32,0x40,0x01 },      0, 244, 0,  0 }, // AVC High 444 Intra
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x50,0x01 },      0, 110, 0,  0 }, // AVC High 10
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x60,0x01 },      0, 122, 0,  0 }, // AVC High 422
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x31,0x70,0x01 },      0, 244, 0,  0 }, // AVC High 444
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x20,0x01 },      0, 110, 0,  1 }, // AVC High 10 Intra
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x01 }, 232960, 110, 1,  1 }, // AVC High 10 Intra RP2027 Class 50 1080/59.94i
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x02 }, 281088, 110, 1,  1 }, // AVC High 10 Intra RP2027 Class 50 1080/50i
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x03 }, 232960, 110, 0,  1 }, // AVC High 10 Intra RP2027 Class 50 1080/29.97p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x04 }, 281088, 110, 0,  1 }, // AVC High 10 Intra RP2027 Class 50 1080/25p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x08 }, 116736, 110, 0,  1 }, // AVC High 10 Intra RP2027 Class 50 720/59.94p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x21,0x09 }, 140800, 110, 0,  1 }, // AVC High 10 Intra RP2027 Class 50 720/50p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x30,0x01 },      0, 122, 0,  1 }, // AVC High 422 Intra
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x01 }, 472576, 122, 1,  1 }, // AVC High 422 Intra RP2027 Class 100 1080/59.94i
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x02 }, 568832, 122, 1,  1 }, // AVC High 422 Intra RP2027 Class 100 1080/50i
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x03 }, 472576, 122, 0,  1 }, // AVC High 422 Intra RP2027 Class 100 1080/29.97p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x04 }, 568832, 122, 0,  1 }, // AVC High 422 Intra RP2027 Class 100 1080/25p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x08 }, 236544, 122, 0,  1 }, // AVC High 422 Intra RP2027 Class 100 720/59.94p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x01,0x32,0x31,0x09 }, 284672, 122, 0,  1 }, // AVC High 422 Intra RP2027 Class 100 720/50p
+    {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x01,0x32,0x40,0x01 },      0, 244, 0,  1 }, // AVC High 444 Intra
     {{ 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x01,0x32,0x50,0x01 },      0,  44, 0, -1 }, // AVC CAVLC 444
 };
 
@@ -2168,37 +2227,81 @@  static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
 {
     MXFContext *mxf = s->priv_data;
     MXFStreamContext *sc = st->priv_data;
-    AVCodecParameters *par = st->codecpar;
-    static const int mxf_h264_num_codec_uls = sizeof(mxf_h264_codec_uls) / sizeof(mxf_h264_codec_uls[0]);
+    H264ParamSets ps = { { 0 } };
+    GetBitContext gb;
     const uint8_t *buf = pkt->data;
     const uint8_t *buf_end = pkt->data + pkt->size;
+    const uint8_t *tmp, *nal_end;
     uint32_t state = -1;
-    int long_gop = 0; // assume intra when there is no SPS header
     int extra_size = 512; // support AVC Intra files without SPS/PPS header
-    int i, frame_size;
-    uint8_t uid_found;
-
-    if (pkt->size > extra_size)
-        buf_end -= pkt->size - extra_size; // no need to parse beyond SPS/PPS header
+    int i, j, tmp_size, frame_size, ret, slice_type, intra_only = 0;
 
     for (;;) {
         buf = avpriv_find_start_code(buf, buf_end, &state);
         if (buf >= buf_end)
             break;
-        --buf;
+
         switch (state & 0x1f) {
         case H264_NAL_SPS:
-            par->profile = buf[1];
-            long_gop = buf[2] & 0x10 ? 0 : 1; // constraint_set3_flag signals intra
             e->flags |= 0x40;
-            break;
+
+            if (ps.sps != NULL)
+                break;
+
+            nal_end = ff_avc_find_startcode(buf, buf_end);
+            tmp = ff_nal_unit_extract_rbsp(buf, nal_end - buf, &tmp_size, 0);
+            if (!buf) {
+                av_log(s, AV_LOG_ERROR, "error extracting rbsp\n");
+                return 0;
+            }
+            init_get_bits(&gb, tmp, tmp_size*8);
+            ret = avpriv_h264_decode_seq_parameter_set(&gb, (AVCodecContext*)s, &ps, 0);
+            av_freep(&tmp);
+            if (ret < 0)
+                return 0;
+            for (j = 0; j < MAX_SPS_COUNT; j++) {
+                if (ps.sps_list[j] != NULL) {
+                    ps.sps = (SPS*)ps.sps_list[j]->data;
+                    break;
+                }
+            }
+
+            sc->aspect_ratio.num = st->codecpar->width * ps.sps->sar.num;
+            sc->aspect_ratio.den = st->codecpar->height * ps.sps->sar.den;
+            av_reduce(&sc->aspect_ratio.num, &sc->aspect_ratio.den,
+                      sc->aspect_ratio.num, sc->aspect_ratio.den, 1024*1024);
+            intra_only = (ps.sps->constraint_set_flags >> 3) & 1;
+            sc->interlaced = !ps.sps->frame_mbs_only_flag;
+            sc->component_depth = ps.sps->bit_depth_luma;
+
+            buf = nal_end;
+            continue;
         case H264_NAL_PPS:
             if (e->flags & 0x40) { // sequence header present
                 e->flags |= 0x80; // random access
                 extra_size = 0;
-                buf = buf_end;
             }
             break;
+        case H264_NAL_IDR_SLICE:
+            e->flags |= 0x04; // IDR Picture
+            buf = buf_end;
+            break;
+        case H264_NAL_SLICE:
+            init_get_bits(&gb, buf, buf_end - buf);
+            get_ue_golomb_long(&gb); // skip first_mb_in_slice
+            slice_type = get_ue_golomb_31(&gb);
+            switch (slice_type % 5) {
+            case 0:
+                e->flags |= 0x20; // P Picture
+                e->flags |= 0x06; // P Picture
+                break;
+            case 1:
+                e->flags |= 0x30; // B Picture
+                e->flags |= 0x03; // non-referenced B Picture
+                break;
+            }
+            buf = buf_end;
+            break;
         default:
             break;
         }
@@ -2207,27 +2310,35 @@  static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
     if (mxf->header_written)
         return 1;
 
-    sc->aspect_ratio = (AVRational){ 16, 9 }; // 16:9 is mandatory for broadcast HD
-    sc->interlaced = par->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0;
-
-    uid_found = 0;
+    if (!ps.sps)
+        sc->interlaced = st->codecpar->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0;
+    sc->codec_ul = NULL;
     frame_size = pkt->size + extra_size;
-    for (i = 0; i < mxf_h264_num_codec_uls; i++) {
+
+    for (i = 0; i < FF_ARRAY_ELEMS(mxf_h264_codec_uls); i++) {
         if (frame_size == mxf_h264_codec_uls[i].frame_size && sc->interlaced == mxf_h264_codec_uls[i].interlaced) {
             sc->codec_ul = &mxf_h264_codec_uls[i].uid;
             sc->component_depth = 10; // AVC Intra is always 10 Bit
+            sc->aspect_ratio = (AVRational){ 16, 9 }; // 16:9 is mandatory for broadcast HD
+            st->codecpar->profile = mxf_h264_codec_uls[i].profile;
+            sc->avc_intra = 1;
             if (sc->interlaced)
                 sc->field_dominance = 1; // top field first is mandatory for AVC Intra
+            mxf->cbr_index = 1;
+            sc->frame_size = frame_size;
             return 1;
-        } else if ((mxf_h264_codec_uls[i].profile == par->profile) &&
-                   ((mxf_h264_codec_uls[i].long_gop < 0) ||
-                   (mxf_h264_codec_uls[i].long_gop == long_gop))) {
+        } else if (ps.sps && mxf_h264_codec_uls[i].frame_size == 0 &&
+                   mxf_h264_codec_uls[i].profile == ps.sps->profile_idc &&
+                   (mxf_h264_codec_uls[i].intra_only < 0 ||
+                    mxf_h264_codec_uls[i].intra_only == intra_only)) {
             sc->codec_ul = &mxf_h264_codec_uls[i].uid;
-            uid_found = 1;
+            st->codecpar->profile = ps.sps->profile_idc;
+            st->codecpar->level = ps.sps->level_idc;
+            // continue to check for avc intra
         }
     }
 
-    if (!uid_found) {
+    if (!sc->codec_ul) {
         av_log(s, AV_LOG_ERROR, "h264 profile not supported\n");
         return 0;
     }