[FFmpeg-devel] [PATCH 2/7] avcodec/golomb: Make emitting error message in get_ue_golomb optional

[James Almer]

On 7/14/2020 12:34 PM, Andreas Rheinhardt wrote:
> This is designed for scenarios where the caller already checks that the
> returned value is within a certain allowed range and returns an error
> message if not.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavcodec/golomb.h | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
> index 5bfcfe085f..63069f63e5 100644
> --- a/libavcodec/golomb.h
> +++ b/libavcodec/golomb.h
> @@ -47,12 +47,7 @@ extern const uint8_t ff_interleaved_ue_golomb_vlc_code[256];
>  extern const  int8_t ff_interleaved_se_golomb_vlc_code[256];
>  extern const uint8_t ff_interleaved_dirac_golomb_vlc_code[256];
>  
> -/**
> - * Read an unsigned Exp-Golomb code in the range 0 to 8190.
> - *
> - * @returns the read value or a negative error code.
> - */
> -static inline int get_ue_golomb(GetBitContext *gb)
> +static inline int get_ue_golomb_internal(GetBitContext *gb, int emit_error_msg)
>  {
>      unsigned int buf;
>  
> @@ -67,7 +62,8 @@ static inline int get_ue_golomb(GetBitContext *gb)
>      } else {
>          int log = 2 * av_log2(buf) - 31;
>          if (log < 0) {
> -            av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
> +            if (emit_error_msg)
> +                av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
>              return AVERROR_INVALIDDATA;
>          }
>          buf >>= log;
> @@ -92,7 +88,8 @@ static inline int get_ue_golomb(GetBitContext *gb)
>          LAST_SKIP_BITS(re, gb, 32 - log);
>          CLOSE_READER(re, gb);
>          if (log < 7) {
> -            av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
> +            if (emit_error_msg)
> +                av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
>              return AVERROR_INVALIDDATA;
>          }
>          buf >>= log;
> @@ -103,6 +100,25 @@ static inline int get_ue_golomb(GetBitContext *gb)
>  #endif
>  }
>  
> +/**
> + * Read an unsigned Exp-Golomb code in the range 0 to 8190.
> + *
> + * @returns the read value or a negative error code.
> + */
> +static inline int get_ue_golomb(GetBitContext *gb)
> +{
> +    return get_ue_golomb_internal(gb, 1);
> +}
> +
> +/**
> + * Variant of get_ue_golomb that does not emit an error message
> + * if the number is outside the permissible range.
> + */
> +static inline int get_ue_golomb2(GetBitContext *gb)

Seeing there's precedent for functions where the number suffix relates
to the range of values they can parse, i think using a 2 here could be
confusing at first glance. I suggest to use something else.

Is it worth keeping the error messages at all, for that matter? They
don't even use a proper logging context, so the printed output is ugly
and unidentifiable by itself.

> +{
> +    return get_ue_golomb_internal(gb, 0);
> +}
> +
>  /**
>   * Read an unsigned Exp-Golomb code in the range 0 to UINT32_MAX-1.
>   */
>