[FFmpeg-devel] [PATCH 1/2] avcodec/h264_parse: change prefix to avpriv for usage in avformat mxf muxer

[Hendrik Leppkes]

On Wed, Apr 10, 2019 at 12:44 AM Mark Thompson <sw at jkqxz.net> wrote:
>
> On 09/04/2019 23:14, Baptiste Coudurier wrote:
> > ---
> >  libavcodec/h264_parse.c  | 2 +-
> >  libavcodec/h264_parser.c | 2 +-
> >  libavcodec/h264_ps.c     | 4 ++--
> >  libavcodec/h264_ps.h     | 4 ++--
> >  libavcodec/h264dec.c     | 6 +++---
> >  5 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > index 17bfa780ce..980b1e189d 100644
> > --- a/libavcodec/h264_ps.c
> > +++ b/libavcodec/h264_ps.c
> > @@ -330,8 +330,8 @@ void ff_h264_ps_uninit(H264ParamSets *ps)
> >      ps->sps = NULL;
> >  }
> >
> > -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
> > -                                     H264ParamSets *ps, int ignore_truncation)
> > +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
> > +                                         H264ParamSets *ps, int ignore_truncation)
> >  {
> >      AVBufferRef *sps_buf;
> >      int profile_idc, level_idc, constraint_set_flags = 0;
> > diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h
> > index e967b9cbcf..d422ce122e 100644
> > --- a/libavcodec/h264_ps.h
> > +++ b/libavcodec/h264_ps.h
> > @@ -149,8 +149,8 @@ typedef struct H264ParamSets {
> >  /**
> >   * Decode SPS
> >   */
> > -int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
> > -                                     H264ParamSets *ps, int ignore_truncation);
> > +int avpriv_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
> > +                                         H264ParamSets *ps, int ignore_truncation);
>
> Making the H.264 decoder's internal SPS and PPS structures fixed API really doesn't feel like a good idea to me, but I admit I don't have a better answer to the problem you're facing.  Copying everything is also pretty terrible.  Adding a new API to parse the headers and return the information you want might be nicer considering just this change, but extensibility for the inevitable subsequent patch which wants some extra piece of information in future would be a big pain.
>
> If you're going to go with this, please add namespace prefixes to the structures it affects ("SPS" and "PPS", since you're now including them in code which isn't explicitly H.264), and also add big warnings to the header indicating that the structures are now fixed and can't be modified at all except on major bumps.  (Though maybe wait for other comments before actually making any changes.)
>

I agree that I really don't like exporting these functions. For
example, this function takes an AVCodecContext. Its reasonable to
assume when touching this function right now that its a valid
AVCodecContext with a valid H264Context inside of it. Apparently right
now its only used for logging, so some crazy cast appears to work, but
in the future?
Internal decoder functionality has no place being exported, it puts
too many limits in place for future changes.

Either the API needs to be wrapped in a proper external interface,
leaving our internal interface open to changes, or a simplified
version should be implemented in avformat, like we already did for a
bunch of other video formats as it was required. The SPS isn't that
complex tbh, so maybe implementing a simplified parser for mxf might
be the best option.

- Hendrik