diff mbox series

[FFmpeg-devel,10/13] avcodec/cinepakenc: Check all calls to avpriv_elbg_do()

Message ID AM7PR03MB666029AE47C9B49D3B5E60AD8FDD9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit ad27326e2b51ba2566bd7b72bea8e8031c2bc4c5
Headers show
Series [FFmpeg-devel,01/13] avcodec/elbg: Remove avoidable buffer | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 17, 2021, 2:08 a.m. UTC 
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/cinepakenc.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Paul B Mahol Sept. 17, 2021, 6:51 a.m. UTC | #1 
lgtm
Tomas Härdin Sept. 17, 2021, 7:46 a.m. UTC | #2 
fre 2021-09-17 klockan 04:08 +0200 skrev Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/cinepakenc.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)

Gave this one a try with -vframes 100 -s 640x360 for some random clips
on my machine. Wall time doesn't change much.

/Tomas
Andreas Rheinhardt Sept. 20, 2021, 9:36 p.m. UTC | #3 
Tomas Härdin:
> fre 2021-09-17 klockan 04:08 +0200 skrev Andreas Rheinhardt:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/cinepakenc.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> Gave this one a try with -vframes 100 -s 640x360 for some random clips
> on my machine. Wall time doesn't change much.
> 

I highly doubted that this would be even noticeable, but your comment
made me benchmark this. This patch is indeed not noticeable, but patches
6 and 7 are a bit noticeable. They even outweigh the benefits of
avoiding the allocations. I guess you might be seeing this.
The reason seems to be that the compiler now has to presume that the
ELBGContext is aliased (whereas before that it was just a structure on
the stack of which it could prove that its address doesn't leak). Adding
av_restrict fixed this (with GCC 10) and improved performance to
something a bit better than it was before this patchset. They also help
for the msvideo1 case (where the speedup due to avoiding allocations
dwarfs everything else).
Sorry for only benchmarking the msvideo1 case earlier and not noticing this.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c
index 2984b93de3..d1bcf2b2d5 100644
--- a/libavcodec/cinepakenc.c
+++ b/libavcodec/cinepakenc.c
@@ -709,6 +709,7 @@  static int quantize(CinepakEncContext *s, int h, uint8_t *data[4],
     uint8_t vq_pict_buf[(MB_AREA * 3) / 2];
     uint8_t     *sub_data[4],     *vq_data[4];
     int      sub_linesize[4],  vq_linesize[4];
+    int ret;
 
     for (mbn = i = y = 0; y < h; y += MB_SIZE) {
         for (x = 0; x < s->w; x += MB_SIZE, ++mbn) {
@@ -762,8 +763,10 @@  static int quantize(CinepakEncContext *s, int h, uint8_t *data[4],
     if (i < size)
         size = i;
 
-    avpriv_elbg_do(&s->elbg, s->codebook_input, entry_size, i, codebook,
-                   size, 1, s->codebook_closest, &s->randctx);
+    ret = avpriv_elbg_do(&s->elbg, s->codebook_input, entry_size, i, codebook,
+                         size, 1, s->codebook_closest, &s->randctx);
+    if (ret < 0)
+        return ret;
 
     // set up vq_data, which contains a single MB
     vq_data[0]     = vq_pict_buf;
@@ -888,8 +891,10 @@  static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
                 if (mode == MODE_V1_ONLY) {
                     info.v1_size = v1_size;
                     // the size may shrink even before optimizations if the input is short:
-                    info.v1_size = quantize(s, h, data, linesize, 1,
-                                            &info, ENC_UNCERTAIN);
+                    if ((new_v1_size = quantize(s, h, data, linesize, 1,
+                                                &info, ENC_UNCERTAIN)) < 0)
+                        return new_v1_size;
+                    info.v1_size = new_v1_size;
                     if (info.v1_size < v1_size)
                         // too few eligible blocks, no sense in trying bigger sizes
                         v1enough = 1;
@@ -902,8 +907,11 @@  static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
 
                     if (mode == MODE_V1_V4) {
                         info.v4_size = v4_size;
-                        info.v4_size = quantize(s, h, data, linesize, 0,
-                                                &info, ENC_UNCERTAIN);
+                        new_v4_size = quantize(s, h, data, linesize, 0,
+                                               &info, ENC_UNCERTAIN);
+                        if (new_v4_size < 0)
+                            return new_v4_size;
+                        info.v4_size = new_v4_size;
                         if (info.v4_size < v4_size)
                             // too few eligible blocks, no sense in trying bigger sizes
                             v4enough = 1;
@@ -921,11 +929,15 @@  static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
                     // we assume we _may_ come here with more blocks to encode than before
                     info.v1_size = v1_size;
                     new_v1_size = quantize(s, h, data, linesize, 1, &info, ENC_V1);
+                    if (new_v1_size < 0)
+                        return new_v1_size;
                     if (new_v1_size < info.v1_size)
                         info.v1_size = new_v1_size;
                     // we assume we _may_ come here with more blocks to encode than before
                     info.v4_size = v4_size;
                     new_v4_size = quantize(s, h, data, linesize, 0, &info, ENC_V4);
+                    if (new_v4_size < 0)
+                        return new_v4_size;
                     if (new_v4_size < info.v4_size)
                         info.v4_size = new_v4_size;
                     // calculate the resulting score
@@ -942,12 +954,16 @@  static int rd_strip(CinepakEncContext *s, int y, int h, int keyframe,
                         if (v1shrunk) {
                             info.v1_size = v1_size;
                             new_v1_size = quantize(s, h, data, linesize, 1, &info, ENC_V1);
+                            if (new_v1_size < 0)
+                                return new_v1_size;
                             if (new_v1_size < info.v1_size)
                                 info.v1_size = new_v1_size;
                         }
                         if (v4shrunk) {
                             info.v4_size = v4_size;
                             new_v4_size = quantize(s, h, data, linesize, 0, &info, ENC_V4);
+                            if (new_v4_size < 0)
+                                return new_v4_size;
                             if (new_v4_size < info.v4_size)
                                 info.v4_size = new_v4_size;
                         }