diff mbox series

[FFmpeg-devel,4/4] avcodec/flashsv2enc: factorize updating block dimensions

Message ID 20210110012045.20448-4-cus@passwd.hu
State Accepted
Commit 257a83b969157eb76c18158a4e503e908d8b1125
Headers show
Series [FFmpeg-devel,1/4] avformat/swfenc: fix generation of FileAttributes tag
Related show

Checks

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

Commit Message

Marton Balint Jan. 10, 2021, 1:20 a.m. UTC
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavcodec/flashsv2enc.c | 76 +++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

Comments

Anton Khirnov Jan. 18, 2021, 11:13 a.m. UTC | #1
Quoting Marton Balint (2021-01-10 02:20:45)
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavcodec/flashsv2enc.c | 76 +++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
> index 6603d0ded1..5139b17a28 100644
> --- a/libavcodec/flashsv2enc.c
> +++ b/libavcodec/flashsv2enc.c
> @@ -252,7 +259,7 @@ static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
>      s->use_custom_palette =  0;
>      s->palette_type       = -1;        // so that the palette will be generated in reconfigure_at_keyframe
>  
> -    return 0;
> +    return update_block_dimensions(s, 64, 64);

This looks different from the original value. Why the change?
Marton Balint Jan. 18, 2021, 8:53 p.m. UTC | #2
On Mon, 18 Jan 2021, Anton Khirnov wrote:

> Quoting Marton Balint (2021-01-10 02:20:45)
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavcodec/flashsv2enc.c | 76 +++++++++++++++++-----------------------
>>  1 file changed, 32 insertions(+), 44 deletions(-)
>> 
>> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
>> index 6603d0ded1..5139b17a28 100644
>> --- a/libavcodec/flashsv2enc.c
>> +++ b/libavcodec/flashsv2enc.c
>> @@ -252,7 +259,7 @@ static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
>>      s->use_custom_palette =  0;
>>      s->palette_type       = -1;        // so that the palette will be generated in reconfigure_at_keyframe
>> 
>> -    return 0;
>> +    return update_block_dimensions(s, 64, 64);
>
> This looks different from the original value. Why the change?

Block dimensions are recalculated on every keyframe, so it does not 
matter what the block width/height is after init. 64 is used 
because the current code always selects that (see the 
optimum_block_width/height functions).

Regards,
Marton
Anton Khirnov Jan. 19, 2021, 11:01 a.m. UTC | #3
Quoting Marton Balint (2021-01-18 21:53:30)
> 
> 
> On Mon, 18 Jan 2021, Anton Khirnov wrote:
> 
> > Quoting Marton Balint (2021-01-10 02:20:45)
> >> Signed-off-by: Marton Balint <cus@passwd.hu>
> >> ---
> >>  libavcodec/flashsv2enc.c | 76 +++++++++++++++++-----------------------
> >>  1 file changed, 32 insertions(+), 44 deletions(-)
> >> 
> >> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
> >> index 6603d0ded1..5139b17a28 100644
> >> --- a/libavcodec/flashsv2enc.c
> >> +++ b/libavcodec/flashsv2enc.c
> >> @@ -252,7 +259,7 @@ static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
> >>      s->use_custom_palette =  0;
> >>      s->palette_type       = -1;        // so that the palette will be generated in reconfigure_at_keyframe
> >> 
> >> -    return 0;
> >> +    return update_block_dimensions(s, 64, 64);
> >
> > This looks different from the original value. Why the change?
> 
> Block dimensions are recalculated on every keyframe, so it does not 
> matter what the block width/height is after init. 64 is used 
> because the current code always selects that (see the 
> optimum_block_width/height functions).

Okay, makes sense. You could mention it in the commit message, since
one would not expect a 'factorize' commit to change behaviour.
Marton Balint Jan. 23, 2021, 9:13 p.m. UTC | #4
On Tue, 19 Jan 2021, Anton Khirnov wrote:

> Quoting Marton Balint (2021-01-18 21:53:30)
>> 
>> 
>> On Mon, 18 Jan 2021, Anton Khirnov wrote:
>> 
>> > Quoting Marton Balint (2021-01-10 02:20:45)
>> >> Signed-off-by: Marton Balint <cus@passwd.hu>
>> >> ---
>> >>  libavcodec/flashsv2enc.c | 76 +++++++++++++++++-----------------------
>> >>  1 file changed, 32 insertions(+), 44 deletions(-)
>> >> 
>> >> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
>> >> index 6603d0ded1..5139b17a28 100644
>> >> --- a/libavcodec/flashsv2enc.c
>> >> +++ b/libavcodec/flashsv2enc.c
>> >> @@ -252,7 +259,7 @@ static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
>> >>      s->use_custom_palette =  0;
>> >>      s->palette_type       = -1;        // so that the palette will be generated in reconfigure_at_keyframe
>> >> 
>> >> -    return 0;
>> >> +    return update_block_dimensions(s, 64, 64);
>> >
>> > This looks different from the original value. Why the change?
>> 
>> Block dimensions are recalculated on every keyframe, so it does not 
>> matter what the block width/height is after init. 64 is used 
>> because the current code always selects that (see the 
>> optimum_block_width/height functions).
>
> Okay, makes sense. You could mention it in the commit message, since
> one would not expect a 'factorize' commit to change behaviour.

OK, applied the series with the comment added.

Thanks,
Marton
James Almer Jan. 27, 2021, 1:13 a.m. UTC | #5
On 1/23/2021 6:13 PM, Marton Balint wrote:
> 
> 
> On Tue, 19 Jan 2021, Anton Khirnov wrote:
> 
>> Quoting Marton Balint (2021-01-18 21:53:30)
>>>
>>>
>>> On Mon, 18 Jan 2021, Anton Khirnov wrote:
>>>
>>> > Quoting Marton Balint (2021-01-10 02:20:45)
>>> >> Signed-off-by: Marton Balint <cus@passwd.hu>
>>> >> ---
>>> >>  libavcodec/flashsv2enc.c | 76 
>>> +++++++++++++++++-----------------------
>>> >>  1 file changed, 32 insertions(+), 44 deletions(-)
>>> >> >> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
>>> >> index 6603d0ded1..5139b17a28 100644
>>> >> --- a/libavcodec/flashsv2enc.c
>>> >> +++ b/libavcodec/flashsv2enc.c
>>> >> @@ -252,7 +259,7 @@ static av_cold int 
>>> flashsv2_encode_init(AVCodecContext * avctx)
>>> >>      s->use_custom_palette =  0;
>>> >>      s->palette_type       = -1;        // so that the palette 
>>> will be generated in reconfigure_at_keyframe
>>> >> >> -    return 0;
>>> >> +    return update_block_dimensions(s, 64, 64);
>>> >
>>> > This looks different from the original value. Why the change?
>>>
>>> Block dimensions are recalculated on every keyframe, so it does not 
>>> matter what the block width/height is after init. 64 is used because 
>>> the current code always selects that (see the 
>>> optimum_block_width/height functions).
>>
>> Okay, makes sense. You could mention it in the commit message, since
>> one would not expect a 'factorize' commit to change behaviour.
> 
> OK, applied the series with the comment added.
> 
> Thanks,
> Marton

Valgrind doesn't like this patch.

http://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-valgrind&time=20210126180022
diff mbox series

Patch

diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
index 6603d0ded1..5139b17a28 100644
--- a/libavcodec/flashsv2enc.c
+++ b/libavcodec/flashsv2enc.c
@@ -174,6 +174,33 @@  static void reset_stats(FlashSV2Context * s)
 #endif
 }
 
+static int update_block_dimensions(FlashSV2Context *s, int block_width, int block_height)
+{
+    s->block_width  = block_width;
+    s->block_height = block_height;
+    s->rows = (s->image_height + s->block_height - 1) / s->block_height;
+    s->cols = (s->image_width  + s->block_width  - 1) / s->block_width;
+    if (s->rows * s->cols > s->blocks_size / sizeof(Block)) {
+        s->frame_blocks = av_realloc_array(s->frame_blocks, s->rows, s->cols * sizeof(Block));
+        s->key_blocks = av_realloc_array(s->key_blocks, s->cols, s->rows * sizeof(Block));
+        if (!s->frame_blocks || !s->key_blocks) {
+            av_log(s->avctx, AV_LOG_ERROR, "Memory allocation failed.\n");
+            return AVERROR(ENOMEM);
+        }
+        s->blocks_size = s->rows * s->cols * sizeof(Block);
+    }
+    init_blocks(s, s->frame_blocks, s->encbuffer, s->databuffer);
+    init_blocks(s, s->key_blocks, s->keybuffer, 0);
+
+    av_fast_malloc(&s->blockbuffer, &s->blockbuffer_size, block_width * block_height * 6);
+    if (!s->blockbuffer) {
+        av_log(s->avctx, AV_LOG_ERROR, "Could not allocate block buffer.\n");
+        return AVERROR(ENOMEM);
+    }
+    return 0;
+}
+
+
 static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
 {
     FlashSV2Context *s = avctx->priv_data;
@@ -211,39 +238,19 @@  static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
     s->image_width  = avctx->width;
     s->image_height = avctx->height;
 
-    s->block_width  = (s->image_width /  12) & ~15;
-    s->block_height = (s->image_height / 12) & ~15;
-
-    if(!s->block_width)
-        s->block_width = 1;
-    if(!s->block_height)
-        s->block_height = 1;
-
-    s->rows = (s->image_height + s->block_height - 1) / s->block_height;
-    s->cols = (s->image_width +  s->block_width -  1) / s->block_width;
-
     s->frame_size  = s->image_width * s->image_height * 3;
-    s->blocks_size = s->rows * s->cols * sizeof(Block);
 
     s->encbuffer     = av_mallocz(s->frame_size);
     s->keybuffer     = av_mallocz(s->frame_size);
     s->databuffer    = av_mallocz(s->frame_size * 6);
     s->current_frame = av_mallocz(s->frame_size);
     s->key_frame     = av_mallocz(s->frame_size);
-    s->frame_blocks  = av_mallocz(s->blocks_size);
-    s->key_blocks    = av_mallocz(s->blocks_size);
     if (!s->encbuffer || !s->keybuffer || !s->databuffer
-        || !s->current_frame || !s->key_frame || !s->key_blocks
-        || !s->frame_blocks) {
+        || !s->current_frame || !s->key_frame) {
         av_log(avctx, AV_LOG_ERROR, "Memory allocation failed.\n");
         return AVERROR(ENOMEM);
     }
 
-    s->blockbuffer      = NULL;
-    s->blockbuffer_size = 0;
-
-    init_blocks(s, s->frame_blocks, s->encbuffer, s->databuffer);
-    init_blocks(s, s->key_blocks,   s->keybuffer, 0);
     reset_stats(s);
 #ifndef FLASHSV2_DUMB
     s->total_bits = 1;
@@ -252,7 +259,7 @@  static av_cold int flashsv2_encode_init(AVCodecContext * avctx)
     s->use_custom_palette =  0;
     s->palette_type       = -1;        // so that the palette will be generated in reconfigure_at_keyframe
 
-    return 0;
+    return update_block_dimensions(s, 64, 64);
 }
 
 static int new_key_frame(FlashSV2Context * s)
@@ -800,29 +807,10 @@  static int reconfigure_at_keyframe(FlashSV2Context * s, const uint8_t * image,
     int block_width  = optimum_block_width (s);
     int block_height = optimum_block_height(s);
 
-    s->rows = (s->image_height + block_height - 1) / block_height;
-    s->cols = (s->image_width  + block_width  - 1) / block_width;
-
     if (block_width != s->block_width || block_height != s->block_height) {
-        s->block_width  = block_width;
-        s->block_height = block_height;
-        if (s->rows * s->cols > s->blocks_size / sizeof(Block)) {
-            s->frame_blocks = av_realloc_array(s->frame_blocks, s->rows, s->cols * sizeof(Block));
-            s->key_blocks = av_realloc_array(s->key_blocks, s->cols, s->rows * sizeof(Block));
-            if (!s->frame_blocks || !s->key_blocks) {
-                av_log(s->avctx, AV_LOG_ERROR, "Memory allocation failed.\n");
-                return -1;
-            }
-            s->blocks_size = s->rows * s->cols * sizeof(Block);
-        }
-        init_blocks(s, s->frame_blocks, s->encbuffer, s->databuffer);
-        init_blocks(s, s->key_blocks, s->keybuffer, 0);
-
-        av_fast_malloc(&s->blockbuffer, &s->blockbuffer_size, block_width * block_height * 6);
-        if (!s->blockbuffer) {
-            av_log(s->avctx, AV_LOG_ERROR, "Could not allocate block buffer.\n");
-            return AVERROR(ENOMEM);
-        }
+        res = update_block_dimensions(s, block_width, block_height);
+        if (res < 0)
+            return res;
     }
 
     s->use15_7 = optimum_use15_7(s);