Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec/magicyuv: add vlc multi support

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 8182 bytes --]

On Wed, Sep 06, 2023 at 10:19:27PM +0000, Paul B Mahol wrote:
> ffmpeg | branch: master | Paul B Mahol <onemda@gmail.com> | Mon Aug 28 12:20:15 2023 +0200| [8b7391cb5ff94ce94612fda69392a95d7ab1ffd0] | committer: Paul B Mahol
> 
> avcodec/magicyuv: add vlc multi support
> 
> Gives nice speed boost, depending on encoded content it goes from
> 30% to 60% faster.
> 
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=8b7391cb5ff94ce94612fda69392a95d7ab1ffd0
> ---
> 
>  libavcodec/magicyuv.c | 65 +++++++++++++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c
> index 7898cd5be4..bbaf14d0e0 100644
> --- a/libavcodec/magicyuv.c
> +++ b/libavcodec/magicyuv.c
> @@ -34,6 +34,8 @@
>  #include "lossless_videodsp.h"
>  #include "thread.h"
>  
> +#define VLC_BITS 12
> +
>  typedef struct Slice {
>      uint32_t start;
>      uint32_t size;
> @@ -67,13 +69,14 @@ typedef struct MagicYUVContext {
>      Slice            *slices[4];      // slice bitstream positions for each plane
>      unsigned int      slices_size[4]; // slice sizes for each plane
>      VLC               vlc[4];         // VLC for each plane
> +    VLC_MULTI         multi[4];       // Buffer for joint VLC data
>      int (*magy_decode_slice)(AVCodecContext *avctx, void *tdata,
>                               int j, int threadnr);
>      LLVidDSPContext   llviddsp;
>  } MagicYUVContext;
>  
>  static int huff_build(const uint8_t len[], uint16_t codes_pos[33],
> -                      VLC *vlc, int nb_elems, void *logctx)
> +                      VLC *vlc, VLC_MULTI *multi, int nb_elems, void *logctx)
>  {
>      HuffEntry he[4096];
>  
> @@ -84,7 +87,8 @@ static int huff_build(const uint8_t len[], uint16_t codes_pos[33],
>          he[--codes_pos[len[i]]] = (HuffEntry){ len[i], i };
>  
>      ff_free_vlc(vlc);
> -    return ff_init_vlc_from_lengths(vlc, FFMIN(he[0].len, 12), nb_elems,
> +    ff_free_vlc_multi(multi);
> +    return ff_init_vlc_multi_from_lengths(vlc, multi, FFMIN(he[0].len, VLC_BITS), nb_elems, nb_elems,
>                                      &he[0].len, sizeof(he[0]),
>                                      &he[0].sym, sizeof(he[0]), sizeof(he[0].sym),
>                                      0, 0, logctx);
> @@ -111,6 +115,22 @@ static void magicyuv_median_pred16(uint16_t *dst, const uint16_t *src1,
>      *left_top = lt;
>  }
>  
> +#define READ_PLANE(dst, plane, b, c) \
> +{ \
> +    x = 0; \
> +    for (; CACHED_BITSTREAM_READER && x < width-c && get_bits_left(&gb) > 0;) {\
> +        ret = get_vlc_multi(&gb, (uint8_t *)dst + x * b, multi, \
> +                            vlc, vlc_bits, 3); \
> +        if (ret > 0) \
> +            x += ret; \
> +        if (ret <= 0) \
> +            return AVERROR_INVALIDDATA; \
> +    } \
> +    for (; x < width && get_bits_left(&gb) > 0; x++) \
> +        dst[x] = get_vlc2(&gb, vlc, vlc_bits, 3); \
> +    dst += stride; \
> +}
> +
>  static int magy_decode_slice10(AVCodecContext *avctx, void *tdata,
>                                 int j, int threadnr)
>  {
> @@ -130,6 +150,9 @@ static int magy_decode_slice10(AVCodecContext *avctx, void *tdata,
>          int sheight = AV_CEIL_RSHIFT(s->slice_height, s->vshift[i]);
>          ptrdiff_t fake_stride = (p->linesize[i] / 2) * (1 + interlaced);
>          ptrdiff_t stride = p->linesize[i] / 2;
> +        const VLC_MULTI_ELEM *const multi = s->multi[i].table;
> +        const VLCElem *const vlc = s->vlc[i].table;
> +        const int vlc_bits = s->vlc[i].bits;
>          int flags, pred;
>          int ret = init_get_bits8(&gb, s->buf + s->slices[i][j].start,
>                                   s->slices[i][j].size);
> @@ -151,20 +174,8 @@ static int magy_decode_slice10(AVCodecContext *avctx, void *tdata,
>                  dst += stride;
>              }
>          } else {
> -            for (k = 0; k < height; k++) {
> -                for (x = 0; x < width; x++) {
> -                    int pix;
> -                    if (get_bits_left(&gb) <= 0)
> -                        return AVERROR_INVALIDDATA;
> -
> -                    pix = get_vlc2(&gb, s->vlc[i].table, s->vlc[i].bits, 3);
> -                    if (pix < 0)
> -                        return AVERROR_INVALIDDATA;
> -
> -                    dst[x] = pix;
> -                }
> -                dst += stride;
> -            }
> +            for (k = 0; k < height; k++)
> +                READ_PLANE(dst, i, 2, 3)
>          }
>  
>          switch (pred) {
> @@ -261,6 +272,9 @@ static int magy_decode_slice(AVCodecContext *avctx, void *tdata,
>          ptrdiff_t fake_stride = p->linesize[i] * (1 + interlaced);
>          ptrdiff_t stride = p->linesize[i];
>          const uint8_t *slice = s->buf + s->slices[i][j].start;
> +        const VLC_MULTI_ELEM *const multi = s->multi[i].table;
> +        const VLCElem *const vlc = s->vlc[i].table;
> +        const int vlc_bits = s->vlc[i].bits;
>          int flags, pred;
>  
>          flags = bytestream_get_byte(&slice);
> @@ -280,20 +294,8 @@ static int magy_decode_slice(AVCodecContext *avctx, void *tdata,
>              if (ret < 0)
>                  return ret;
>  
> -            for (k = 0; k < height; k++) {
> -                for (x = 0; x < width; x++) {
> -                    int pix;
> -                    if (get_bits_left(&gb) <= 0)
> -                        return AVERROR_INVALIDDATA;
> -
> -                    pix = get_vlc2(&gb, s->vlc[i].table, s->vlc[i].bits, 3);
> -                    if (pix < 0)
> -                        return AVERROR_INVALIDDATA;
> -
> -                    dst[x] = pix;
> -                }
> -                dst += stride;
> -            }
> +            for (k = 0; k < height; k++)
> +                READ_PLANE(dst, i, 1, 5)
>          }
>  
>          switch (pred) {

Who reviewed this ?

This is a straight out of array write
writing 8 bytes while the check assumes its max 5

==16861==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x631000024800 at pc 0x000001552992 bp 0x7ffd8d34bf10 sp 0x7ffd8d34bf08
WRITE of size 8 at 0x631000024800 thread T0
    #0 0x1552991 in bits_read_vlc_multi_be ffmpeg/libavcodec/bitstream_template.h:531:9
    #1 0x15453d0 in magy_decode_slice ffmpeg/libavcodec/magicyuv.c:302:17
    #2 0x13f3738 in avcodec_default_execute2 ffmpeg/libavcodec/avcodec.c:76:17
    #3 0x153fba5 in magy_decode_frame ffmpeg/libavcodec/magicyuv.c:663:5
    #4 0x142d60e in decode_simple_internal ffmpeg/libavcodec/decode.c:430:20
    #5 0x142d60e in decode_simple_receive_frame ffmpeg/libavcodec/decode.c:609
    #6 0x142d60e in decode_receive_frame_internal ffmpeg/libavcodec/decode.c:637
    #7 0x142c42f in avcodec_send_packet ffmpeg/libavcodec/decode.c:734:15
    #8 0x4ce9df in LLVMFuzzerTestOneInput ffmpeg/tools/target_dec_fuzzer.c:579:25
    #9 0x29f5f0d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
    #10 0x29eaae2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
    #11 0x29efce1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
    #12 0x29ea7c0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
    #13 0x7f15a835ec86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #14 0x420339 in _start (ffmpeg/tools/target_dec_magicyuv_fuzzer+0x420339)


the multi vlc code is not documented, teh function writing out of array
is one of the very few functions in bitstream_template.h that is undocumented

why do we in 2023 add undocumented non trivial functions into files where basically
every function prior is documented ?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".