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

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

Details

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

Commit Message

Baptiste Coudurier April 9, 2019, 10:14 p.m.
---
 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(-)

Comments

Mark Thompson April 9, 2019, 10:43 p.m.
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.)

- Mark
Hendrik Leppkes April 9, 2019, 11:11 p.m.
On Wed, Apr 10, 2019 at 12:44 AM Mark Thompson <sw@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

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
index a075443d17..0f008c29f3 100644
--- a/libavcodec/h264_parse.c
+++ b/libavcodec/h264_parse.c
@@ -374,7 +374,7 @@  static int decode_extradata_ps(const uint8_t *data, int size, H264ParamSets *ps,
         H2645NAL *nal = &pkt.nals[i];
         switch (nal->type) {
         case H264_NAL_SPS:
-            ret = ff_h264_decode_seq_parameter_set(&nal->gb, logctx, ps, 0);
+            ret = avpriv_h264_decode_seq_parameter_set(&nal->gb, logctx, ps, 0);
             if (ret < 0)
                 goto fail;
             break;
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 5f9a9c46ef..814584cc82 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -323,7 +323,7 @@  static inline int parse_nal_units(AVCodecParserContext *s,
 
         switch (nal.type) {
         case H264_NAL_SPS:
-            ff_h264_decode_seq_parameter_set(&nal.gb, avctx, &p->ps, 0);
+            avpriv_h264_decode_seq_parameter_set(&nal.gb, avctx, &p->ps, 0);
             break;
         case H264_NAL_PPS:
             ff_h264_decode_picture_parameter_set(&nal.gb, avctx, &p->ps,
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);
 
 /**
  * Decode PPS
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 837c3b7538..2e82e118b7 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -716,14 +716,14 @@  static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
                 if (ret < 0)
                     goto end;
             }
-            if (ff_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0)
+            if (avpriv_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0)
                 break;
             av_log(h->avctx, AV_LOG_DEBUG,
                    "SPS decoding failure, trying again with the complete NAL\n");
             init_get_bits8(&tmp_gb, nal->raw_data + 1, nal->raw_size - 1);
-            if (ff_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0)
+            if (avpriv_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0)
                 break;
-            ff_h264_decode_seq_parameter_set(&nal->gb, avctx, &h->ps, 1);
+            avpriv_h264_decode_seq_parameter_set(&nal->gb, avctx, &h->ps, 1);
             break;
         }
         case H264_NAL_PPS: