diff mbox series

[FFmpeg-devel,18/18] h264_ps: pass AVCodecContext as void* where possible

Message ID 20200313102850.23913-18-anton@khirnov.net
State Accepted
Headers show
Series [FFmpeg-devel,01/18] mpeg4videodec: do not copy a range of fields at once
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork warning Failed to apply patch

Commit Message

Anton Khirnov March 13, 2020, 10:28 a.m. UTC
Makes sure it is only used for logging and nothing else.
---
 libavcodec/h264_ps.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer March 13, 2020, 10:29 p.m. UTC | #1
On Fri, Mar 13, 2020 at 11:28:50AM +0100, Anton Khirnov wrote:
> Makes sure it is only used for logging and nothing else.
> ---
>  libavcodec/h264_ps.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> index 1951bb1161..4ef25aa514 100644
> --- a/libavcodec/h264_ps.c
> +++ b/libavcodec/h264_ps.c
> @@ -104,14 +104,14 @@ static void remove_sps(H264ParamSets *s, int id)
>      av_buffer_unref(&s->sps_list[id]);
>  }
>  
> -static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
> +static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,

this is a double sided sword
while fields of logctx cannot be used its after this possible to pass
wrong things as logctx

[...]
Anton Khirnov March 15, 2020, 5:02 p.m. UTC | #2
Quoting Michael Niedermayer (2020-03-13 23:29:12)
> On Fri, Mar 13, 2020 at 11:28:50AM +0100, Anton Khirnov wrote:
> > Makes sure it is only used for logging and nothing else.
> > ---
> >  libavcodec/h264_ps.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > index 1951bb1161..4ef25aa514 100644
> > --- a/libavcodec/h264_ps.c
> > +++ b/libavcodec/h264_ps.c
> > @@ -104,14 +104,14 @@ static void remove_sps(H264ParamSets *s, int id)
> >      av_buffer_unref(&s->sps_list[id]);
> >  }
> >  
> > -static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
> > +static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
> 
> this is a double sided sword
> while fields of logctx cannot be used its after this possible to pass
> wrong things as logctx

Right, but that should be easily noticeable since it will crash on
dereferencing the AVClass. I consider the danger of people accessing the
AVCodecContext inappropriately to be bigger (since it's done in many
places already).

But we might want to consider something like
typedef AVClass* AVLogger
Michael Niedermayer March 16, 2020, 11:23 a.m. UTC | #3
On Sun, Mar 15, 2020 at 06:02:04PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-03-13 23:29:12)
> > On Fri, Mar 13, 2020 at 11:28:50AM +0100, Anton Khirnov wrote:
> > > Makes sure it is only used for logging and nothing else.
> > > ---
> > >  libavcodec/h264_ps.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > > index 1951bb1161..4ef25aa514 100644
> > > --- a/libavcodec/h264_ps.c
> > > +++ b/libavcodec/h264_ps.c
> > > @@ -104,14 +104,14 @@ static void remove_sps(H264ParamSets *s, int id)
> > >      av_buffer_unref(&s->sps_list[id]);
> > >  }
> > >  
> > > -static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
> > > +static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
> > 
> > this is a double sided sword
> > while fields of logctx cannot be used its after this possible to pass
> > wrong things as logctx
> 
> Right, but that should be easily noticeable since it will crash on
> dereferencing the AVClass. I consider the danger of people accessing the
> AVCodecContext inappropriately to be bigger (since it's done in many
> places already).
> 

> But we might want to consider something like
> typedef AVClass* AVLogger

yes, if that ends up looking clean in practice then iam certainly in favor

thx

[...]
Anton Khirnov March 18, 2020, 9:02 a.m. UTC | #4
Quoting Michael Niedermayer (2020-03-16 12:23:11)
> On Sun, Mar 15, 2020 at 06:02:04PM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2020-03-13 23:29:12)
> > > On Fri, Mar 13, 2020 at 11:28:50AM +0100, Anton Khirnov wrote:
> > > > Makes sure it is only used for logging and nothing else.
> > > > ---
> > > >  libavcodec/h264_ps.c | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > > > index 1951bb1161..4ef25aa514 100644
> > > > --- a/libavcodec/h264_ps.c
> > > > +++ b/libavcodec/h264_ps.c
> > > > @@ -104,14 +104,14 @@ static void remove_sps(H264ParamSets *s, int id)
> > > >      av_buffer_unref(&s->sps_list[id]);
> > > >  }
> > > >  
> > > > -static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
> > > > +static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
> > > 
> > > this is a double sided sword
> > > while fields of logctx cannot be used its after this possible to pass
> > > wrong things as logctx
> > 
> > Right, but that should be easily noticeable since it will crash on
> > dereferencing the AVClass. I consider the danger of people accessing the
> > AVCodecContext inappropriately to be bigger (since it's done in many
> > places already).
> > 
> 
> > But we might want to consider something like
> > typedef AVClass* AVLogger
> 
> yes, if that ends up looking clean in practice then iam certainly in favor

That would require changes to the logging API though, so would be
outside of the scope of this set.
Are you objecting to the patch as it is?
Michael Niedermayer March 23, 2020, 5:12 p.m. UTC | #5
On Wed, Mar 18, 2020 at 10:02:39AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-03-16 12:23:11)
> > On Sun, Mar 15, 2020 at 06:02:04PM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2020-03-13 23:29:12)
> > > > On Fri, Mar 13, 2020 at 11:28:50AM +0100, Anton Khirnov wrote:
> > > > > Makes sure it is only used for logging and nothing else.
> > > > > ---
> > > > >  libavcodec/h264_ps.c | 18 +++++++++---------
> > > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> > > > > index 1951bb1161..4ef25aa514 100644
> > > > > --- a/libavcodec/h264_ps.c
> > > > > +++ b/libavcodec/h264_ps.c
> > > > > @@ -104,14 +104,14 @@ static void remove_sps(H264ParamSets *s, int id)
> > > > >      av_buffer_unref(&s->sps_list[id]);
> > > > >  }
> > > > >  
> > > > > -static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
> > > > > +static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
> > > > 
> > > > this is a double sided sword
> > > > while fields of logctx cannot be used its after this possible to pass
> > > > wrong things as logctx
> > > 
> > > Right, but that should be easily noticeable since it will crash on
> > > dereferencing the AVClass. I consider the danger of people accessing the
> > > AVCodecContext inappropriately to be bigger (since it's done in many
> > > places already).
> > > 
> > 
> > > But we might want to consider something like
> > > typedef AVClass* AVLogger
> > 
> > yes, if that ends up looking clean in practice then iam certainly in favor
> 
> That would require changes to the logging API though, so would be
> outside of the scope of this set.
> Are you objecting to the patch as it is?

no i dont really mind, iam fine with the code as it is before as well as after
the patch. Its better in one way worse in another. So really no reason for me
to be against this if someone else has a preferrance

Thanks

[...]
diff mbox series

Patch

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 1951bb1161..4ef25aa514 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -104,14 +104,14 @@  static void remove_sps(H264ParamSets *s, int id)
     av_buffer_unref(&s->sps_list[id]);
 }
 
-static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx,
+static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
                                         SPS *sps)
 {
     int cpb_count, i;
     cpb_count = get_ue_golomb_31(gb) + 1;
 
     if (cpb_count > 32U) {
-        av_log(avctx, AV_LOG_ERROR, "cpb_count %d invalid\n", cpb_count);
+        av_log(logctx, AV_LOG_ERROR, "cpb_count %d invalid\n", cpb_count);
         return AVERROR_INVALIDDATA;
     }
 
@@ -130,7 +130,7 @@  static inline int decode_hrd_parameters(GetBitContext *gb, AVCodecContext *avctx
     return 0;
 }
 
-static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx,
+static inline int decode_vui_parameters(GetBitContext *gb, void *logctx,
                                         SPS *sps)
 {
     int aspect_ratio_info_present_flag;
@@ -146,7 +146,7 @@  static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
         } else if (aspect_ratio_idc < FF_ARRAY_ELEMS(ff_h264_pixel_aspect)) {
             sps->sar = ff_h264_pixel_aspect[aspect_ratio_idc];
         } else {
-            av_log(avctx, AV_LOG_ERROR, "illegal aspect ratio\n");
+            av_log(logctx, AV_LOG_ERROR, "illegal aspect ratio\n");
             return AVERROR_INVALIDDATA;
         }
     } else {
@@ -187,7 +187,7 @@  static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
         sps->chroma_location = AVCHROMA_LOC_LEFT;
 
     if (show_bits1(gb) && get_bits_left(gb) < 10) {
-        av_log(avctx, AV_LOG_WARNING, "Truncated VUI (%d)\n", get_bits_left(gb));
+        av_log(logctx, AV_LOG_WARNING, "Truncated VUI (%d)\n", get_bits_left(gb));
         return 0;
     }
 
@@ -196,7 +196,7 @@  static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
         unsigned num_units_in_tick = get_bits_long(gb, 32);
         unsigned time_scale        = get_bits_long(gb, 32);
         if (!num_units_in_tick || !time_scale) {
-            av_log(avctx, AV_LOG_ERROR,
+            av_log(logctx, AV_LOG_ERROR,
                    "time_scale/num_units_in_tick invalid or unsupported (%u/%u)\n",
                    time_scale, num_units_in_tick);
             sps->timing_info_present_flag = 0;
@@ -209,11 +209,11 @@  static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
 
     sps->nal_hrd_parameters_present_flag = get_bits1(gb);
     if (sps->nal_hrd_parameters_present_flag)
-        if (decode_hrd_parameters(gb, avctx, sps) < 0)
+        if (decode_hrd_parameters(gb, logctx, sps) < 0)
             return AVERROR_INVALIDDATA;
     sps->vcl_hrd_parameters_present_flag = get_bits1(gb);
     if (sps->vcl_hrd_parameters_present_flag)
-        if (decode_hrd_parameters(gb, avctx, sps) < 0)
+        if (decode_hrd_parameters(gb, logctx, sps) < 0)
             return AVERROR_INVALIDDATA;
     if (sps->nal_hrd_parameters_present_flag ||
         sps->vcl_hrd_parameters_present_flag)
@@ -238,7 +238,7 @@  static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
 
         if (sps->num_reorder_frames > 16U
             /* max_dec_frame_buffering || max_dec_frame_buffering > 16 */) {
-            av_log(avctx, AV_LOG_ERROR,
+            av_log(logctx, AV_LOG_ERROR,
                    "Clipping illegal num_reorder_frames %d\n",
                    sps->num_reorder_frames);
             sps->num_reorder_frames = 16;