[FFmpeg-devel] [PATCH] avcodec/lagarith: use VLC for probe code length

[Andreas Rheinhardt]

Paul B Mahol:
> 
> +#include "libavutil/mem_internal.h"

I don't get what this header is needed for. You are not adding anything
ALIGNED and this file does not require it.

> +#define VLC_BITS 11
> +
>  enum LagarithFrameType {
>      FRAME_RAW           = 1,    /**< uncompressed */
>      FRAME_U_RGB24       = 2,    /**< unaligned RGB24 */
> @@ -56,6 +61,35 @@ typedef struct LagarithContext {
>      int zeros_rem;              /**< number of zero bytes remaining to output */
>  } LagarithContext;
>  
> +static VLC lag_tab;
> +
> +static const uint8_t lag_bits[] = {
> +    7, 7, 7, 2, 7, 3, 4, 5, 6, 7, 7, 7, 7, 7, 6, 7, 4, 5, 7, 7, 7, 7,
> +    5, 6, 7, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 7, 7,
> +    7, 7, 7, 7, 7, 7, 7, 7, 7, 7,
> +};
> +
> +static const uint8_t lag_codes[] = {
> +    0x00, 0x01, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x05,
> +    0x08, 0x09, 0x0A, 0x0B, 0x0B, 0x0B, 0x0B, 0x10, 0x11, 0x12, 0x13,
> +    0x13, 0x13, 0x14, 0x15, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25,
> +    0x28, 0x29, 0x2A, 0x2B, 0x2B, 0x40, 0x41, 0x42, 0x43, 0x44, 0x45,
> +    0x48, 0x49, 0x4A, 0x4B, 0x50, 0x51, 0x52, 0x53, 0x54, 0x55,
> +};
> +
> +static const uint8_t lag_symbols[] = {
> +    -1, 20, 12, 0, 12, 1, 2, 4, 7, 7, 28, 4, 25, 17,
> +    10, 17, 3, 6, 2, 23, 15, 15, 5, 9, 10, 31, 1, 22,
> +    14, 14, 8, 9, 30, 6, 27, 19, 11, 19, 0, 21, 13, 13,
> +    8, 29, 5, 26, 18, 18, 3, 24, 16, 16, 11, 32,
> +};
> +
> +static av_cold void lag_init_static_data(void)
> +{
> +    VLC_INIT_SPARSE_STATIC(&lag_tab, VLC_BITS, FF_ARRAY_ELEMS(lag_bits),
> +                           lag_bits, 1, 1, lag_codes, 1, 1, lag_symbols, 1, 1, 2048);
> +}
> +

If the longest code has seven bits, why are you using 11 bits for the
VLC? This just wastes cache/memory.

Apart from that:
Your first entry will be converted to an uint8_t of 255 (and give a
-Woverflow warning when said warning is enabled) and this is what
get_vlc2() will return for it, i.e. it will trigger the bits > 31 check
and error out, which is probably what you intend it to do given that
this behaviour coincides with the current behaviour.

But the more natural way for VLCs to achieve this is to actually not add
invalid codes. get_vlc2() will then return -1 for them; this means that
the check for invalid values becomes "bits < 0" (in which case the flags
from this comparison can be reused for the "bits == 0" check).
In contrast to the current code and your proposed patch no bits would be
consumed upon encountering such an invalid code though. But it seems to
me that the we error out anyway and the state of the GetBitContext
afterwards doesn't matter.

If you were to use the init_from_lengths variants, you would have to use
negative bitlengths for the invalid values.

- Andreas