diff mbox series

[FFmpeg-devel,36/39] avcodec/speedhqenc: Call correct function

Message ID 20201210111657.2276739-37-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series Make mpegvideo encoders init-threadsafe | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Dec. 10, 2020, 11:16 a.m. UTC 
Up until now, the SpeedHQ encoder called a wrong function for init:
void ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256],
                        uint8_t *uni_ac_vlc_len);
Yet the first argument actually used is of type RLTable; the size of
said struct is less than 256 if the size of a pointer is four, leading
to an access beyond the end of the RLTable.

This commit fixes this by calling the actually intended function:
init_uni_ac_vlc() from mpeg12enc.c. It was intended to use this
function [1], yet doing so was forgotten when the patch was actually
applied.

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-July/266187.html

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/Makefile     |  2 +-
 libavcodec/mpeg12.h     |  1 +
 libavcodec/mpeg12enc.c  | 11 ++++++++---
 libavcodec/speedhqenc.c |  2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

Comments

Paul B Mahol Dec. 10, 2020, 11:39 a.m. UTC | #1 
OK,

I guess encoding still works.

On Thu, Dec 10, 2020 at 12:25 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Up until now, the SpeedHQ encoder called a wrong function for init:
> void ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256],
>                         uint8_t *uni_ac_vlc_len);
> Yet the first argument actually used is of type RLTable; the size of
> said struct is less than 256 if the size of a pointer is four, leading
> to an access beyond the end of the RLTable.
>
> This commit fixes this by calling the actually intended function:
> init_uni_ac_vlc() from mpeg12enc.c. It was intended to use this
> function [1], yet doing so was forgotten when the patch was actually
> applied.
>
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-July/266187.html
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/Makefile     |  2 +-
>  libavcodec/mpeg12.h     |  1 +
>  libavcodec/mpeg12enc.c  | 11 ++++++++---
>  libavcodec/speedhqenc.c |  2 +-
>  4 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 7f018e313b..450781886d 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -626,7 +626,7 @@ OBJS-$(CONFIG_SONIC_DECODER)           += sonic.o
>  OBJS-$(CONFIG_SONIC_ENCODER)           += sonic.o
>  OBJS-$(CONFIG_SONIC_LS_ENCODER)        += sonic.o
>  OBJS-$(CONFIG_SPEEDHQ_DECODER)         += speedhq.o mpeg12.o mpeg12data.o
> simple_idct.o
> -OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o
> speedhqenc.o
> +OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o
> mpeg12enc.o speedhqenc.o
>  OBJS-$(CONFIG_SP5X_DECODER)            += sp5xdec.o
>  OBJS-$(CONFIG_SRGC_DECODER)            += mscc.o
>  OBJS-$(CONFIG_SRT_DECODER)             += srtdec.o ass.o htmlsubtitles.o
> diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h
> index 76fc0bf955..4cd48b5d20 100644
> --- a/libavcodec/mpeg12.h
> +++ b/libavcodec/mpeg12.h
> @@ -35,6 +35,7 @@ void ff_mpeg12_common_init(MpegEncContext *s);
>  }
>
>  void ff_init_2d_vlc_rl(RLTable *rl, unsigned static_size, int flags);
> +void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t *uni_ac_vlc_len);
>
>  static inline int decode_dc(GetBitContext *gb, int component)
>  {
> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> index e38cd074e1..a05c2db6cb 100644
> --- a/libavcodec/mpeg12enc.c
> +++ b/libavcodec/mpeg12enc.c
> @@ -27,6 +27,7 @@
>
>  #include <stdint.h>
>
> +#include "config.h"
>  #include "libavutil/attributes.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/log.h"
> @@ -44,6 +45,7 @@
>  #include "mpegvideo.h"
>  #include "profiles.h"
>
> +#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
>  static const uint8_t svcd_scan_offset_placeholder[] = {
>      0x10, 0x0E, 0x00, 0x80, 0x81, 0x00, 0x80,
>      0x81, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> @@ -61,8 +63,9 @@ static uint32_t mpeg1_lum_dc_uni[512];
>  static uint32_t mpeg1_chr_dc_uni[512];
>
>  #define A53_MAX_CC_COUNT 0x1f
> +#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
>
> -static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len)
> +av_cold void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t
> *uni_ac_vlc_len)
>  {
>      int i;
>
> @@ -97,6 +100,7 @@ static av_cold void init_uni_ac_vlc(RLTable *rl,
> uint8_t *uni_ac_vlc_len)
>      }
>  }
>
> +#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
>  static int find_frame_rate_index(MpegEncContext *s)
>  {
>      int i;
> @@ -1039,8 +1043,8 @@ static av_cold void mpeg12_encode_init_static(void)
>      ff_rl_init(&ff_rl_mpeg1, mpeg12_static_rl_table_store[0]);
>      ff_rl_init(&ff_rl_mpeg2, mpeg12_static_rl_table_store[1]);
>
> -    init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
> -    init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
>
>      /* build unified dc encoding tables */
>      for (int i = -255; i < 256; i++) {
> @@ -1216,3 +1220,4 @@ AVCodec ff_mpeg2video_encoder = {
>      .caps_internal        = FF_CODEC_CAP_INIT_THREADSAFE |
> FF_CODEC_CAP_INIT_CLEANUP,
>      .priv_class           = &mpeg2_class,
>  };
> +#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
> diff --git a/libavcodec/speedhqenc.c b/libavcodec/speedhqenc.c
> index 9807024980..a5bedd5301 100644
> --- a/libavcodec/speedhqenc.c
> +++ b/libavcodec/speedhqenc.c
> @@ -96,7 +96,7 @@ static av_cold void speedhq_init_static_data(void)
>          speedhq_chr_dc_uni[i + 255] = bits + (code << 8);
>      }
>
> -    ff_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
>  }
>
>  av_cold int ff_speedhq_encode_init(MpegEncContext *s)
> --
> 2.25.1
>
> _______________________________________________
> 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".
Andreas Rheinhardt Dec. 10, 2020, 11:26 p.m. UTC | #2 
Paul B Mahol:
> OK,
> 
> I guess encoding still works.
> 

Yes, it does. In fact, you need to enable either trellis or
quantizer_noise_shaping for it to make a difference. A quick test with
trellis on showed strange artifacts without this patch that are
eliminated by this patch; furthermore, the output is now reproducible.
So I'll apply this.

- Andreas

> On Thu, Dec 10, 2020 at 12:25 PM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Up until now, the SpeedHQ encoder called a wrong function for init:
>> void ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256],
>>                         uint8_t *uni_ac_vlc_len);
>> Yet the first argument actually used is of type RLTable; the size of
>> said struct is less than 256 if the size of a pointer is four, leading
>> to an access beyond the end of the RLTable.
>>
>> This commit fixes this by calling the actually intended function:
>> init_uni_ac_vlc() from mpeg12enc.c. It was intended to use this
>> function [1], yet doing so was forgotten when the patch was actually
>> applied.
>>
>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-July/266187.html
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/Makefile     |  2 +-
>>  libavcodec/mpeg12.h     |  1 +
>>  libavcodec/mpeg12enc.c  | 11 ++++++++---
>>  libavcodec/speedhqenc.c |  2 +-
>>  4 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 7f018e313b..450781886d 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -626,7 +626,7 @@ OBJS-$(CONFIG_SONIC_DECODER)           += sonic.o
>>  OBJS-$(CONFIG_SONIC_ENCODER)           += sonic.o
>>  OBJS-$(CONFIG_SONIC_LS_ENCODER)        += sonic.o
>>  OBJS-$(CONFIG_SPEEDHQ_DECODER)         += speedhq.o mpeg12.o mpeg12data.o
>> simple_idct.o
>> -OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o
>> speedhqenc.o
>> +OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o
>> mpeg12enc.o speedhqenc.o
>>  OBJS-$(CONFIG_SP5X_DECODER)            += sp5xdec.o
>>  OBJS-$(CONFIG_SRGC_DECODER)            += mscc.o
>>  OBJS-$(CONFIG_SRT_DECODER)             += srtdec.o ass.o htmlsubtitles.o
>> diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h
>> index 76fc0bf955..4cd48b5d20 100644
>> --- a/libavcodec/mpeg12.h
>> +++ b/libavcodec/mpeg12.h
>> @@ -35,6 +35,7 @@ void ff_mpeg12_common_init(MpegEncContext *s);
>>  }
>>
>>  void ff_init_2d_vlc_rl(RLTable *rl, unsigned static_size, int flags);
>> +void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t *uni_ac_vlc_len);
>>
>>  static inline int decode_dc(GetBitContext *gb, int component)
>>  {
>> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
>> index e38cd074e1..a05c2db6cb 100644
>> --- a/libavcodec/mpeg12enc.c
>> +++ b/libavcodec/mpeg12enc.c
>> @@ -27,6 +27,7 @@
>>
>>  #include <stdint.h>
>>
>> +#include "config.h"
>>  #include "libavutil/attributes.h"
>>  #include "libavutil/avassert.h"
>>  #include "libavutil/log.h"
>> @@ -44,6 +45,7 @@
>>  #include "mpegvideo.h"
>>  #include "profiles.h"
>>
>> +#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
>>  static const uint8_t svcd_scan_offset_placeholder[] = {
>>      0x10, 0x0E, 0x00, 0x80, 0x81, 0x00, 0x80,
>>      0x81, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>> @@ -61,8 +63,9 @@ static uint32_t mpeg1_lum_dc_uni[512];
>>  static uint32_t mpeg1_chr_dc_uni[512];
>>
>>  #define A53_MAX_CC_COUNT 0x1f
>> +#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
>>
>> -static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len)
>> +av_cold void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t
>> *uni_ac_vlc_len)
>>  {
>>      int i;
>>
>> @@ -97,6 +100,7 @@ static av_cold void init_uni_ac_vlc(RLTable *rl,
>> uint8_t *uni_ac_vlc_len)
>>      }
>>  }
>>
>> +#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
>>  static int find_frame_rate_index(MpegEncContext *s)
>>  {
>>      int i;
>> @@ -1039,8 +1043,8 @@ static av_cold void mpeg12_encode_init_static(void)
>>      ff_rl_init(&ff_rl_mpeg1, mpeg12_static_rl_table_store[0]);
>>      ff_rl_init(&ff_rl_mpeg2, mpeg12_static_rl_table_store[1]);
>>
>> -    init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
>> -    init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
>> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
>> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
>>
>>      /* build unified dc encoding tables */
>>      for (int i = -255; i < 256; i++) {
>> @@ -1216,3 +1220,4 @@ AVCodec ff_mpeg2video_encoder = {
>>      .caps_internal        = FF_CODEC_CAP_INIT_THREADSAFE |
>> FF_CODEC_CAP_INIT_CLEANUP,
>>      .priv_class           = &mpeg2_class,
>>  };
>> +#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
>> diff --git a/libavcodec/speedhqenc.c b/libavcodec/speedhqenc.c
>> index 9807024980..a5bedd5301 100644
>> --- a/libavcodec/speedhqenc.c
>> +++ b/libavcodec/speedhqenc.c
>> @@ -96,7 +96,7 @@ static av_cold void speedhq_init_static_data(void)
>>          speedhq_chr_dc_uni[i + 255] = bits + (code << 8);
>>      }
>>
>> -    ff_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
>> +    ff_mpeg1_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
>>  }
>>
>>  av_cold int ff_speedhq_encode_init(MpegEncContext *s)
>> --
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 7f018e313b..450781886d 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -626,7 +626,7 @@  OBJS-$(CONFIG_SONIC_DECODER)           += sonic.o
 OBJS-$(CONFIG_SONIC_ENCODER)           += sonic.o
 OBJS-$(CONFIG_SONIC_LS_ENCODER)        += sonic.o
 OBJS-$(CONFIG_SPEEDHQ_DECODER)         += speedhq.o mpeg12.o mpeg12data.o simple_idct.o
-OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o speedhqenc.o
+OBJS-$(CONFIG_SPEEDHQ_ENCODER)         += speedhq.o mpeg12data.o mpeg12enc.o speedhqenc.o
 OBJS-$(CONFIG_SP5X_DECODER)            += sp5xdec.o
 OBJS-$(CONFIG_SRGC_DECODER)            += mscc.o
 OBJS-$(CONFIG_SRT_DECODER)             += srtdec.o ass.o htmlsubtitles.o
diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h
index 76fc0bf955..4cd48b5d20 100644
--- a/libavcodec/mpeg12.h
+++ b/libavcodec/mpeg12.h
@@ -35,6 +35,7 @@  void ff_mpeg12_common_init(MpegEncContext *s);
 }
 
 void ff_init_2d_vlc_rl(RLTable *rl, unsigned static_size, int flags);
+void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t *uni_ac_vlc_len);
 
 static inline int decode_dc(GetBitContext *gb, int component)
 {
diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
index e38cd074e1..a05c2db6cb 100644
--- a/libavcodec/mpeg12enc.c
+++ b/libavcodec/mpeg12enc.c
@@ -27,6 +27,7 @@ 
 
 #include <stdint.h>
 
+#include "config.h"
 #include "libavutil/attributes.h"
 #include "libavutil/avassert.h"
 #include "libavutil/log.h"
@@ -44,6 +45,7 @@ 
 #include "mpegvideo.h"
 #include "profiles.h"
 
+#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
 static const uint8_t svcd_scan_offset_placeholder[] = {
     0x10, 0x0E, 0x00, 0x80, 0x81, 0x00, 0x80,
     0x81, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
@@ -61,8 +63,9 @@  static uint32_t mpeg1_lum_dc_uni[512];
 static uint32_t mpeg1_chr_dc_uni[512];
 
 #define A53_MAX_CC_COUNT 0x1f
+#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
 
-static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len)
+av_cold void ff_mpeg1_init_uni_ac_vlc(const RLTable *rl, uint8_t *uni_ac_vlc_len)
 {
     int i;
 
@@ -97,6 +100,7 @@  static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len)
     }
 }
 
+#if CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER
 static int find_frame_rate_index(MpegEncContext *s)
 {
     int i;
@@ -1039,8 +1043,8 @@  static av_cold void mpeg12_encode_init_static(void)
     ff_rl_init(&ff_rl_mpeg1, mpeg12_static_rl_table_store[0]);
     ff_rl_init(&ff_rl_mpeg2, mpeg12_static_rl_table_store[1]);
 
-    init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
-    init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
+    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg1, uni_mpeg1_ac_vlc_len);
+    ff_mpeg1_init_uni_ac_vlc(&ff_rl_mpeg2, uni_mpeg2_ac_vlc_len);
 
     /* build unified dc encoding tables */
     for (int i = -255; i < 256; i++) {
@@ -1216,3 +1220,4 @@  AVCodec ff_mpeg2video_encoder = {
     .caps_internal        = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP,
     .priv_class           = &mpeg2_class,
 };
+#endif /* CONFIG_MPEG1VIDEO_ENCODER || CONFIG_MPEG2VIDEO_ENCODER */
diff --git a/libavcodec/speedhqenc.c b/libavcodec/speedhqenc.c
index 9807024980..a5bedd5301 100644
--- a/libavcodec/speedhqenc.c
+++ b/libavcodec/speedhqenc.c
@@ -96,7 +96,7 @@  static av_cold void speedhq_init_static_data(void)
         speedhq_chr_dc_uni[i + 255] = bits + (code << 8);
     }
 
-    ff_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
+    ff_mpeg1_init_uni_ac_vlc(&ff_rl_speedhq, uni_speedhq_ac_vlc_len);
 }
 
 av_cold int ff_speedhq_encode_init(MpegEncContext *s)