diff mbox series

[FFmpeg-devel,17/57] avcodec/mpegvideo, mpegpicture: Add buffer pool

Message ID AS8P250MB0744A8359CB929AECAA88AF68F1B2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State Superseded
Headers show
Series [FFmpeg-devel,01/14] avcodec/get_buffer: Remove redundant check | expand

Commit Message

Andreas Rheinhardt April 29, 2024, 9:13 p.m. UTC
This avoids constant allocations+frees and will also allow
to simply switch to the RefStruct API, thereby avoiding
the overhead of the AVBuffer API.
It also simplifies the code, because it removes the "needs_realloc"
field: It was added in 435c0b87d28b48dc2e0360adc404a0e2d66d16a0,
before the introduction of the AVBuffer API: given that these buffers
may be used by different threads, they were not freed immediately
and instead were marked as being freed later by setting needs_realloc.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegpicture.c   | 155 ++++++++-----------------------------
 libavcodec/mpegpicture.h   |  27 ++++---
 libavcodec/mpegvideo.c     |  37 +++++++++
 libavcodec/mpegvideo.h     |   2 +
 libavcodec/mpegvideo_dec.c |  35 ++++-----
 libavcodec/mpegvideo_enc.c |  13 ++--
 6 files changed, 112 insertions(+), 157 deletions(-)

Comments

Michael Niedermayer April 30, 2024, 8:40 p.m. UTC | #1
On Mon, Apr 29, 2024 at 11:13:58PM +0200, Andreas Rheinhardt wrote:
> This avoids constant allocations+frees and will also allow
> to simply switch to the RefStruct API, thereby avoiding
> the overhead of the AVBuffer API.
> It also simplifies the code, because it removes the "needs_realloc"
> field: It was added in 435c0b87d28b48dc2e0360adc404a0e2d66d16a0,
> before the introduction of the AVBuffer API: given that these buffers
> may be used by different threads, they were not freed immediately
> and instead were marked as being freed later by setting needs_realloc.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/mpegpicture.c   | 155 ++++++++-----------------------------
>  libavcodec/mpegpicture.h   |  27 ++++---
>  libavcodec/mpegvideo.c     |  37 +++++++++
>  libavcodec/mpegvideo.h     |   2 +
>  libavcodec/mpegvideo_dec.c |  35 ++++-----
>  libavcodec/mpegvideo_enc.c |  13 ++--
>  6 files changed, 112 insertions(+), 157 deletions(-)

This seems to change the output of:

./ffmpeg -y -bitexact -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi && ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -

--- A	2024-04-30 22:01:12.964146819 +0200
+++ B	2024-04-30 22:00:57.407969834 +0200
@@ -38,7 +38,7 @@
 0,         32,         32,        1,   115200, 0x74c44bae
 0,         33,         33,        1,   115200, 0x921c5255
 0,         34,         34,        1,   115200, 0x9a8553a9
-0,         35,         35,        1,   115200, 0x817b6334
+0,         35,         35,        1,   115200, 0x310061fd
 0,         36,         36,        1,   115200, 0x4c9a5f6d
 0,         37,         37,        1,   115200, 0x5ee86279
 0,         38,         38,        1,   115200, 0x04055061
@@ -74,7 +74,7 @@
 0,         68,         68,        1,   115200, 0x49dcbf4e
 0,         69,         69,        1,   115200, 0x1ea1c7d1
 0,         70,         70,        1,   115200, 0xdf77c67b
-0,         71,         71,        1,   115200, 0x33d9d206
+0,         71,         71,        1,   115200, 0x7f6bd16d
 0,         72,         72,        1,   115200, 0x5e37cb3a
 0,         73,         73,        1,   115200, 0x15abcda3
 0,         74,         74,        1,   115200, 0xbf4dcbd4
@@ -86,7 +86,7 @@
 0,         80,         80,        1,   115200, 0x17d1d667
 0,         81,         81,        1,   115200, 0x0c1fdf9c
 0,         82,         82,        1,   115200, 0x7eabde6b
-0,         83,         83,        1,   115200, 0x3bf6e873
+0,         83,         83,        1,   115200, 0xe623e7af
 0,         84,         84,        1,   115200, 0xf480dc82
 0,         85,         85,        1,   115200, 0x5fd6e098
 0,         86,         86,        1,   115200, 0xf520de95
@@ -98,7 +98,7 @@
 0,         92,         92,        1,   115200, 0x34cfe1c2
 0,         93,         93,        1,   115200, 0x1d94e1c3
 0,         94,         94,        1,   115200, 0x6d32e147
-0,         95,         95,        1,   115200, 0x09fbefd0
+0,         95,         95,        1,   115200, 0x7e40ee91
 0,         96,         96,        1,   115200, 0xa5f5eb43
 0,         97,         97,        1,   115200, 0x39b9ec3d
 0,         98,         98,        1,   115200, 0x3256ec18


[...]
James Almer April 30, 2024, 8:58 p.m. UTC | #2
On 4/30/2024 5:40 PM, Michael Niedermayer wrote:
> On Mon, Apr 29, 2024 at 11:13:58PM +0200, Andreas Rheinhardt wrote:
>> This avoids constant allocations+frees and will also allow
>> to simply switch to the RefStruct API, thereby avoiding
>> the overhead of the AVBuffer API.
>> It also simplifies the code, because it removes the "needs_realloc"
>> field: It was added in 435c0b87d28b48dc2e0360adc404a0e2d66d16a0,
>> before the introduction of the AVBuffer API: given that these buffers
>> may be used by different threads, they were not freed immediately
>> and instead were marked as being freed later by setting needs_realloc.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/mpegpicture.c   | 155 ++++++++-----------------------------
>>   libavcodec/mpegpicture.h   |  27 ++++---
>>   libavcodec/mpegvideo.c     |  37 +++++++++
>>   libavcodec/mpegvideo.h     |   2 +
>>   libavcodec/mpegvideo_dec.c |  35 ++++-----
>>   libavcodec/mpegvideo_enc.c |  13 ++--
>>   6 files changed, 112 insertions(+), 157 deletions(-)
> 
> This seems to change the output of:
> 
> ./ffmpeg -y -bitexact -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi && ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -
> 
> --- A	2024-04-30 22:01:12.964146819 +0200
> +++ B	2024-04-30 22:00:57.407969834 +0200
> @@ -38,7 +38,7 @@
>   0,         32,         32,        1,   115200, 0x74c44bae
>   0,         33,         33,        1,   115200, 0x921c5255
>   0,         34,         34,        1,   115200, 0x9a8553a9
> -0,         35,         35,        1,   115200, 0x817b6334
> +0,         35,         35,        1,   115200, 0x310061fd
>   0,         36,         36,        1,   115200, 0x4c9a5f6d
>   0,         37,         37,        1,   115200, 0x5ee86279
>   0,         38,         38,        1,   115200, 0x04055061
> @@ -74,7 +74,7 @@
>   0,         68,         68,        1,   115200, 0x49dcbf4e
>   0,         69,         69,        1,   115200, 0x1ea1c7d1
>   0,         70,         70,        1,   115200, 0xdf77c67b
> -0,         71,         71,        1,   115200, 0x33d9d206
> +0,         71,         71,        1,   115200, 0x7f6bd16d
>   0,         72,         72,        1,   115200, 0x5e37cb3a
>   0,         73,         73,        1,   115200, 0x15abcda3
>   0,         74,         74,        1,   115200, 0xbf4dcbd4
> @@ -86,7 +86,7 @@
>   0,         80,         80,        1,   115200, 0x17d1d667
>   0,         81,         81,        1,   115200, 0x0c1fdf9c
>   0,         82,         82,        1,   115200, 0x7eabde6b
> -0,         83,         83,        1,   115200, 0x3bf6e873
> +0,         83,         83,        1,   115200, 0xe623e7af
>   0,         84,         84,        1,   115200, 0xf480dc82
>   0,         85,         85,        1,   115200, 0x5fd6e098
>   0,         86,         86,        1,   115200, 0xf520de95
> @@ -98,7 +98,7 @@
>   0,         92,         92,        1,   115200, 0x34cfe1c2
>   0,         93,         93,        1,   115200, 0x1d94e1c3
>   0,         94,         94,        1,   115200, 0x6d32e147
> -0,         95,         95,        1,   115200, 0x09fbefd0
> +0,         95,         95,        1,   115200, 0x7e40ee91
>   0,         96,         96,        1,   115200, 0xa5f5eb43
>   0,         97,         97,        1,   115200, 0x39b9ec3d
>   0,         98,         98,        1,   115200, 0x3256ec18

Last time a buffer pool change resulted in changed checksum for some 
frames it was because the decoder or encoder were touching uninitialized 
bytes, something made evident by the buffer coming from a pool (thus 
potentially being recycled).

Can you check if that's the case here? How different is the output?
Andreas Rheinhardt April 30, 2024, 9:06 p.m. UTC | #3
Michael Niedermayer:
> On Mon, Apr 29, 2024 at 11:13:58PM +0200, Andreas Rheinhardt wrote:
>> This avoids constant allocations+frees and will also allow
>> to simply switch to the RefStruct API, thereby avoiding
>> the overhead of the AVBuffer API.
>> It also simplifies the code, because it removes the "needs_realloc"
>> field: It was added in 435c0b87d28b48dc2e0360adc404a0e2d66d16a0,
>> before the introduction of the AVBuffer API: given that these buffers
>> may be used by different threads, they were not freed immediately
>> and instead were marked as being freed later by setting needs_realloc.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/mpegpicture.c   | 155 ++++++++-----------------------------
>>  libavcodec/mpegpicture.h   |  27 ++++---
>>  libavcodec/mpegvideo.c     |  37 +++++++++
>>  libavcodec/mpegvideo.h     |   2 +
>>  libavcodec/mpegvideo_dec.c |  35 ++++-----
>>  libavcodec/mpegvideo_enc.c |  13 ++--
>>  6 files changed, 112 insertions(+), 157 deletions(-)
> 
> This seems to change the output of:
> 
> ./ffmpeg -y -bitexact -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi && ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -
> 
> --- A	2024-04-30 22:01:12.964146819 +0200
> +++ B	2024-04-30 22:00:57.407969834 +0200
> @@ -38,7 +38,7 @@
>  0,         32,         32,        1,   115200, 0x74c44bae
>  0,         33,         33,        1,   115200, 0x921c5255
>  0,         34,         34,        1,   115200, 0x9a8553a9
> -0,         35,         35,        1,   115200, 0x817b6334
> +0,         35,         35,        1,   115200, 0x310061fd
>  0,         36,         36,        1,   115200, 0x4c9a5f6d
>  0,         37,         37,        1,   115200, 0x5ee86279
>  0,         38,         38,        1,   115200, 0x04055061
> @@ -74,7 +74,7 @@
>  0,         68,         68,        1,   115200, 0x49dcbf4e
>  0,         69,         69,        1,   115200, 0x1ea1c7d1
>  0,         70,         70,        1,   115200, 0xdf77c67b
> -0,         71,         71,        1,   115200, 0x33d9d206
> +0,         71,         71,        1,   115200, 0x7f6bd16d
>  0,         72,         72,        1,   115200, 0x5e37cb3a
>  0,         73,         73,        1,   115200, 0x15abcda3
>  0,         74,         74,        1,   115200, 0xbf4dcbd4
> @@ -86,7 +86,7 @@
>  0,         80,         80,        1,   115200, 0x17d1d667
>  0,         81,         81,        1,   115200, 0x0c1fdf9c
>  0,         82,         82,        1,   115200, 0x7eabde6b
> -0,         83,         83,        1,   115200, 0x3bf6e873
> +0,         83,         83,        1,   115200, 0xe623e7af
>  0,         84,         84,        1,   115200, 0xf480dc82
>  0,         85,         85,        1,   115200, 0x5fd6e098
>  0,         86,         86,        1,   115200, 0xf520de95
> @@ -98,7 +98,7 @@
>  0,         92,         92,        1,   115200, 0x34cfe1c2
>  0,         93,         93,        1,   115200, 0x1d94e1c3
>  0,         94,         94,        1,   115200, 0x6d32e147
> -0,         95,         95,        1,   115200, 0x09fbefd0
> +0,         95,         95,        1,   115200, 0x7e40ee91
>  0,         96,         96,        1,   115200, 0xa5f5eb43
>  0,         97,         97,        1,   115200, 0x39b9ec3d
>  0,         98,         98,        1,   115200, 0x3256ec18
> 

The decoder uses mbskip table values that were not properly set/cleaned
before. The issue is fixed if this buffer is reset every time.

- Andreas
Andreas Rheinhardt April 30, 2024, 9:07 p.m. UTC | #4
James Almer:
> On 4/30/2024 5:40 PM, Michael Niedermayer wrote:
>> On Mon, Apr 29, 2024 at 11:13:58PM +0200, Andreas Rheinhardt wrote:
>>> This avoids constant allocations+frees and will also allow
>>> to simply switch to the RefStruct API, thereby avoiding
>>> the overhead of the AVBuffer API.
>>> It also simplifies the code, because it removes the "needs_realloc"
>>> field: It was added in 435c0b87d28b48dc2e0360adc404a0e2d66d16a0,
>>> before the introduction of the AVBuffer API: given that these buffers
>>> may be used by different threads, they were not freed immediately
>>> and instead were marked as being freed later by setting needs_realloc.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>   libavcodec/mpegpicture.c   | 155 ++++++++-----------------------------
>>>   libavcodec/mpegpicture.h   |  27 ++++---
>>>   libavcodec/mpegvideo.c     |  37 +++++++++
>>>   libavcodec/mpegvideo.h     |   2 +
>>>   libavcodec/mpegvideo_dec.c |  35 ++++-----
>>>   libavcodec/mpegvideo_enc.c |  13 ++--
>>>   6 files changed, 112 insertions(+), 157 deletions(-)
>>
>> This seems to change the output of:
>>
>> ./ffmpeg -y -bitexact -i fate-suite/svq3/Vertical400kbit.sorenson3.mov
>> -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1
>> /tmp/out4.avi && ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i
>> /tmp/out4.avi -bitexact -vsync drop -f framecrc -
>>
>> --- A    2024-04-30 22:01:12.964146819 +0200
>> +++ B    2024-04-30 22:00:57.407969834 +0200
>> @@ -38,7 +38,7 @@
>>   0,         32,         32,        1,   115200, 0x74c44bae
>>   0,         33,         33,        1,   115200, 0x921c5255
>>   0,         34,         34,        1,   115200, 0x9a8553a9
>> -0,         35,         35,        1,   115200, 0x817b6334
>> +0,         35,         35,        1,   115200, 0x310061fd
>>   0,         36,         36,        1,   115200, 0x4c9a5f6d
>>   0,         37,         37,        1,   115200, 0x5ee86279
>>   0,         38,         38,        1,   115200, 0x04055061
>> @@ -74,7 +74,7 @@
>>   0,         68,         68,        1,   115200, 0x49dcbf4e
>>   0,         69,         69,        1,   115200, 0x1ea1c7d1
>>   0,         70,         70,        1,   115200, 0xdf77c67b
>> -0,         71,         71,        1,   115200, 0x33d9d206
>> +0,         71,         71,        1,   115200, 0x7f6bd16d
>>   0,         72,         72,        1,   115200, 0x5e37cb3a
>>   0,         73,         73,        1,   115200, 0x15abcda3
>>   0,         74,         74,        1,   115200, 0xbf4dcbd4
>> @@ -86,7 +86,7 @@
>>   0,         80,         80,        1,   115200, 0x17d1d667
>>   0,         81,         81,        1,   115200, 0x0c1fdf9c
>>   0,         82,         82,        1,   115200, 0x7eabde6b
>> -0,         83,         83,        1,   115200, 0x3bf6e873
>> +0,         83,         83,        1,   115200, 0xe623e7af
>>   0,         84,         84,        1,   115200, 0xf480dc82
>>   0,         85,         85,        1,   115200, 0x5fd6e098
>>   0,         86,         86,        1,   115200, 0xf520de95
>> @@ -98,7 +98,7 @@
>>   0,         92,         92,        1,   115200, 0x34cfe1c2
>>   0,         93,         93,        1,   115200, 0x1d94e1c3
>>   0,         94,         94,        1,   115200, 0x6d32e147
>> -0,         95,         95,        1,   115200, 0x09fbefd0
>> +0,         95,         95,        1,   115200, 0x7e40ee91
>>   0,         96,         96,        1,   115200, 0xa5f5eb43
>>   0,         97,         97,        1,   115200, 0x39b9ec3d
>>   0,         98,         98,        1,   115200, 0x3256ec18
> 
> Last time a buffer pool change resulted in changed checksum for some
> frames it was because the decoder or encoder were touching uninitialized
> bytes, something made evident by the buffer coming from a pool (thus
> potentially being recycled).
> 
> Can you check if that's the case here? How different is the output?

The buffer here is initially zeroed, so valgrind will never show any
uninitialized reads.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
index 06c82880a8..bd4ddc6b55 100644
--- a/libavcodec/mpegpicture.c
+++ b/libavcodec/mpegpicture.c
@@ -29,15 +29,11 @@ 
 #include "avcodec.h"
 #include "motion_est.h"
 #include "mpegpicture.h"
-#include "mpegvideo.h"
 #include "refstruct.h"
 #include "threadframe.h"
 
 static void av_noinline free_picture_tables(Picture *pic)
 {
-    pic->alloc_mb_width  =
-    pic->alloc_mb_height = 0;
-
     av_buffer_unref(&pic->mbskip_table_buf);
     av_buffer_unref(&pic->qscale_table_buf);
     av_buffer_unref(&pic->mb_type_buf);
@@ -46,43 +42,9 @@  static void av_noinline free_picture_tables(Picture *pic)
         av_buffer_unref(&pic->motion_val_buf[i]);
         av_buffer_unref(&pic->ref_index_buf[i]);
     }
-}
-
-static int make_table_writable(AVBufferRef **ref)
-{
-    AVBufferRef *old = *ref, *new;
-
-    if (av_buffer_is_writable(old))
-        return 0;
-    new = av_buffer_allocz(old->size);
-    if (!new)
-        return AVERROR(ENOMEM);
-    av_buffer_unref(ref);
-    *ref = new;
-    return 0;
-}
-
-static int make_tables_writable(Picture *pic)
-{
-#define MAKE_WRITABLE(table) \
-do {\
-    int ret = make_table_writable(&pic->table); \
-    if (ret < 0) \
-        return ret; \
-} while (0)
-
-    MAKE_WRITABLE(mbskip_table_buf);
-    MAKE_WRITABLE(qscale_table_buf);
-    MAKE_WRITABLE(mb_type_buf);
-
-    if (pic->motion_val_buf[0]) {
-        for (int i = 0; i < 2; i++) {
-            MAKE_WRITABLE(motion_val_buf[i]);
-            MAKE_WRITABLE(ref_index_buf[i]);
-        }
-    }
 
-    return 0;
+    pic->mb_width  =
+    pic->mb_height = 0;
 }
 
 int ff_mpeg_framesize_alloc(AVCodecContext *avctx, MotionEstContext *me,
@@ -170,38 +132,28 @@  static int handle_pic_linesizes(AVCodecContext *avctx, Picture *pic,
     return 0;
 }
 
-static int alloc_picture_tables(AVCodecContext *avctx, Picture *pic, int encoding, int out_format,
-                                int mb_stride, int mb_width, int mb_height, int b8_stride)
+static int alloc_picture_tables(BufferPoolContext *pools, Picture *pic,
+                                int mb_height)
 {
-    const int big_mb_num    = mb_stride * (mb_height + 1) + 1;
-    const int mb_array_size = mb_stride * mb_height;
-    const int b8_array_size = b8_stride * mb_height * 2;
-    int i;
-
-
-    pic->mbskip_table_buf = av_buffer_allocz(mb_array_size + 2);
-    pic->qscale_table_buf = av_buffer_allocz(big_mb_num + mb_stride);
-    pic->mb_type_buf      = av_buffer_allocz((big_mb_num + mb_stride) *
-                                             sizeof(uint32_t));
-    if (!pic->mbskip_table_buf || !pic->qscale_table_buf || !pic->mb_type_buf)
-        return AVERROR(ENOMEM);
-
-    if (out_format == FMT_H263 || encoding ||
-        (avctx->export_side_data & AV_CODEC_EXPORT_DATA_MVS)) {
-        int mv_size        = 2 * (b8_array_size + 4) * sizeof(int16_t);
-        int ref_index_size = 4 * mb_array_size;
-
-        for (i = 0; mv_size && i < 2; i++) {
-            pic->motion_val_buf[i] = av_buffer_allocz(mv_size);
-            pic->ref_index_buf[i]  = av_buffer_allocz(ref_index_size);
-            if (!pic->motion_val_buf[i] || !pic->ref_index_buf[i])
-                return AVERROR(ENOMEM);
+#define GET_BUFFER(name, idx_suffix) do { \
+    pic->name ## _buf idx_suffix = av_buffer_pool_get(pools->name ## _pool); \
+    if (!pic->name ## _buf idx_suffix) \
+        return AVERROR(ENOMEM); \
+} while (0)
+    GET_BUFFER(mbskip_table,);
+    GET_BUFFER(qscale_table,);
+    GET_BUFFER(mb_type,);
+    if (pools->motion_val_pool) {
+        for (int i = 0; i < 2; i++) {
+            GET_BUFFER(motion_val, [i]);
+            GET_BUFFER(ref_index,  [i]);
         }
     }
+#undef GET_BUFFER
 
-    pic->alloc_mb_width  = mb_width;
-    pic->alloc_mb_height = mb_height;
-    pic->alloc_mb_stride = mb_stride;
+    pic->mb_width  = pools->alloc_mb_width;
+    pic->mb_height = mb_height;
+    pic->mb_stride = pools->alloc_mb_stride;
 
     return 0;
 }
@@ -211,17 +163,11 @@  static int alloc_picture_tables(AVCodecContext *avctx, Picture *pic, int encodin
  * The pixels are allocated/set by calling get_buffer() if shared = 0
  */
 int ff_alloc_picture(AVCodecContext *avctx, Picture *pic, MotionEstContext *me,
-                     ScratchpadContext *sc, int encoding, int out_format,
-                     int mb_stride, int mb_width, int mb_height, int b8_stride,
-                     ptrdiff_t *linesize, ptrdiff_t *uvlinesize)
+                     ScratchpadContext *sc, BufferPoolContext *pools,
+                     int mb_height, ptrdiff_t *linesize, ptrdiff_t *uvlinesize)
 {
     int i, ret;
 
-    if (pic->qscale_table_buf)
-        if (   pic->alloc_mb_width  != mb_width
-            || pic->alloc_mb_height != mb_height)
-            free_picture_tables(pic);
-
     if (handle_pic_linesizes(avctx, pic, me, sc,
                              *linesize, *uvlinesize) < 0)
         return -1;
@@ -229,17 +175,13 @@  int ff_alloc_picture(AVCodecContext *avctx, Picture *pic, MotionEstContext *me,
     *linesize   = pic->f->linesize[0];
     *uvlinesize = pic->f->linesize[1];
 
-    if (!pic->qscale_table_buf)
-        ret = alloc_picture_tables(avctx, pic, encoding, out_format,
-                                   mb_stride, mb_width, mb_height, b8_stride);
-    else
-        ret = make_tables_writable(pic);
+    ret = alloc_picture_tables(pools, pic, mb_height);
     if (ret < 0)
         goto fail;
 
     pic->mbskip_table = pic->mbskip_table_buf->data;
-    pic->qscale_table = pic->qscale_table_buf->data + 2 * mb_stride + 1;
-    pic->mb_type      = (uint32_t*)pic->mb_type_buf->data + 2 * mb_stride + 1;
+    pic->qscale_table = pic->qscale_table_buf->data       + 2 * pic->mb_stride + 1;
+    pic->mb_type      = (uint32_t*)pic->mb_type_buf->data + 2 * pic->mb_stride + 1;
 
     if (pic->motion_val_buf[0]) {
         for (i = 0; i < 2; i++) {
@@ -256,7 +198,6 @@  int ff_alloc_picture(AVCodecContext *avctx, Picture *pic, MotionEstContext *me,
 fail:
     av_log(avctx, AV_LOG_ERROR, "Error allocating a picture.\n");
     ff_mpeg_unref_picture(pic);
-    free_picture_tables(pic);
     return AVERROR(ENOMEM);
 }
 
@@ -271,20 +212,18 @@  void ff_mpeg_unref_picture(Picture *pic)
 
     ff_refstruct_unref(&pic->hwaccel_picture_private);
 
-    if (pic->needs_realloc)
-        free_picture_tables(pic);
+    free_picture_tables(pic);
 
     pic->dummy         = 0;
     pic->field_picture = 0;
     pic->b_frame_score = 0;
-    pic->needs_realloc = 0;
     pic->reference     = 0;
     pic->shared        = 0;
     pic->display_picture_number = 0;
     pic->coded_picture_number   = 0;
 }
 
-int ff_update_picture_tables(Picture *dst, const Picture *src)
+static int update_picture_tables(Picture *dst, const Picture *src)
 {
     int i, ret;
 
@@ -309,9 +248,9 @@  int ff_update_picture_tables(Picture *dst, const Picture *src)
         dst->ref_index[i]  = src->ref_index[i];
     }
 
-    dst->alloc_mb_width  = src->alloc_mb_width;
-    dst->alloc_mb_height = src->alloc_mb_height;
-    dst->alloc_mb_stride = src->alloc_mb_stride;
+    dst->mb_width  = src->mb_width;
+    dst->mb_height = src->mb_height;
+    dst->mb_stride = src->mb_stride;
 
     return 0;
 }
@@ -329,7 +268,7 @@  int ff_mpeg_ref_picture(Picture *dst, Picture *src)
     if (ret < 0)
         goto fail;
 
-    ret = ff_update_picture_tables(dst, src);
+    ret = update_picture_tables(dst, src);
     if (ret < 0)
         goto fail;
 
@@ -339,7 +278,6 @@  int ff_mpeg_ref_picture(Picture *dst, Picture *src)
     dst->dummy                   = src->dummy;
     dst->field_picture           = src->field_picture;
     dst->b_frame_score           = src->b_frame_score;
-    dst->needs_realloc           = src->needs_realloc;
     dst->reference               = src->reference;
     dst->shared                  = src->shared;
     dst->display_picture_number  = src->display_picture_number;
@@ -351,30 +289,14 @@  fail:
     return ret;
 }
 
-static inline int pic_is_unused(Picture *pic)
-{
-    if (!pic->f->buf[0])
-        return 1;
-    if (pic->needs_realloc)
-        return 1;
-    return 0;
-}
-
-static int find_unused_picture(AVCodecContext *avctx, Picture *picture, int shared)
+int ff_find_unused_picture(AVCodecContext *avctx, Picture *picture, int shared)
 {
     int i;
 
-    if (shared) {
         for (i = 0; i < MAX_PICTURE_COUNT; i++) {
             if (!picture[i].f->buf[0])
                 return i;
         }
-    } else {
-        for (i = 0; i < MAX_PICTURE_COUNT; i++) {
-            if (pic_is_unused(&picture[i]))
-                return i;
-        }
-    }
 
     av_log(avctx, AV_LOG_FATAL,
            "Internal error, picture buffer overflow\n");
@@ -393,21 +315,8 @@  static int find_unused_picture(AVCodecContext *avctx, Picture *picture, int shar
     return -1;
 }
 
-int ff_find_unused_picture(AVCodecContext *avctx, Picture *picture, int shared)
-{
-    int ret = find_unused_picture(avctx, picture, shared);
-
-    if (ret >= 0 && ret < MAX_PICTURE_COUNT) {
-        if (picture[ret].needs_realloc) {
-            ff_mpeg_unref_picture(&picture[ret]);
-        }
-    }
-    return ret;
-}
-
 void av_cold ff_mpv_picture_free(Picture *pic)
 {
-    free_picture_tables(pic);
     ff_mpeg_unref_picture(pic);
     av_frame_free(&pic->f);
 }
diff --git a/libavcodec/mpegpicture.h b/libavcodec/mpegpicture.h
index 664c116a47..a0bfd8250f 100644
--- a/libavcodec/mpegpicture.h
+++ b/libavcodec/mpegpicture.h
@@ -23,6 +23,7 @@ 
 
 #include <stdint.h>
 
+#include "libavutil/buffer.h"
 #include "libavutil/frame.h"
 
 #include "avcodec.h"
@@ -41,6 +42,17 @@  typedef struct ScratchpadContext {
     int      linesize;            ///< linesize that the buffers in this context have been allocated for
 } ScratchpadContext;
 
+typedef struct BufferPoolContext {
+    AVBufferPool *mbskip_table_pool;
+    AVBufferPool *qscale_table_pool;
+    AVBufferPool *mb_type_pool;
+    AVBufferPool *motion_val_pool;
+    AVBufferPool *ref_index_pool;
+    int alloc_mb_width;                         ///< mb_width  used to allocate tables
+    int alloc_mb_height;                        ///< mb_height used to allocate tables
+    int alloc_mb_stride;                        ///< mb_stride used to allocate tables
+} BufferPoolContext;
+
 /**
  * Picture.
  */
@@ -63,18 +75,17 @@  typedef struct Picture {
     AVBufferRef *ref_index_buf[2];
     int8_t *ref_index[2];
 
-    int alloc_mb_width;         ///< mb_width used to allocate tables
-    int alloc_mb_height;        ///< mb_height used to allocate tables
-    int alloc_mb_stride;        ///< mb_stride used to allocate tables
-
     /// RefStruct reference for hardware accelerator private data
     void *hwaccel_picture_private;
 
+    int mb_width;               ///< mb_width  of the tables
+    int mb_height;              ///< mb_height of the tables
+    int mb_stride;              ///< mb_stride of the tables
+
     int dummy;                  ///< Picture is a dummy and should not be output
     int field_picture;          ///< whether or not the picture was encoded in separate fields
 
     int b_frame_score;
-    int needs_realloc;          ///< Picture needs to be reallocated (eg due to a frame size change)
 
     int reference;
     int shared;
@@ -87,9 +98,8 @@  typedef struct Picture {
  * Allocate a Picture's accessories, but not the AVFrame's buffer itself.
  */
 int ff_alloc_picture(AVCodecContext *avctx, Picture *pic, MotionEstContext *me,
-                     ScratchpadContext *sc, int encoding, int out_format,
-                     int mb_stride, int mb_width, int mb_height, int b8_stride,
-                     ptrdiff_t *linesize, ptrdiff_t *uvlinesize);
+                     ScratchpadContext *sc, BufferPoolContext *pools,
+                     int mb_height, ptrdiff_t *linesize, ptrdiff_t *uvlinesize);
 
 int ff_mpeg_framesize_alloc(AVCodecContext *avctx, MotionEstContext *me,
                             ScratchpadContext *sc, int linesize);
@@ -98,7 +108,6 @@  int ff_mpeg_ref_picture(Picture *dst, Picture *src);
 void ff_mpeg_unref_picture(Picture *picture);
 
 void ff_mpv_picture_free(Picture *pic);
-int ff_update_picture_tables(Picture *dst, const Picture *src);
 
 int ff_find_unused_picture(AVCodecContext *avctx, Picture *picture, int shared);
 
diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index ce1edca95d..5728f4cee3 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -534,8 +534,19 @@  void ff_mpv_common_defaults(MpegEncContext *s)
     s->slice_context_count   = 1;
 }
 
+static void free_buffer_pools(BufferPoolContext *pools)
+{
+    av_buffer_pool_uninit(&pools->mbskip_table_pool);
+    av_buffer_pool_uninit(&pools->qscale_table_pool);
+    av_buffer_pool_uninit(&pools->mb_type_pool);
+    av_buffer_pool_uninit(&pools->motion_val_pool);
+    av_buffer_pool_uninit(&pools->ref_index_pool);
+    pools->alloc_mb_height = pools->alloc_mb_width = pools->alloc_mb_stride = 0;
+}
+
 int ff_mpv_init_context_frame(MpegEncContext *s)
 {
+    BufferPoolContext *const pools = &s->buffer_pools;
     int y_size, c_size, yc_size, i, mb_array_size, mv_table_size, x, y;
     int mb_height;
 
@@ -630,11 +641,36 @@  int ff_mpv_init_context_frame(MpegEncContext *s)
         return AVERROR(ENOMEM);
     memset(s->mbintra_table, 1, mb_array_size);
 
+#define ALLOC_POOL(name, size) do { \
+    pools->name ##_pool = av_buffer_pool_init((size), av_buffer_allocz); \
+    if (!pools->name ##_pool) \
+        return AVERROR(ENOMEM); \
+} while (0)
+
+    ALLOC_POOL(mbskip_table, mb_array_size + 2);
+    ALLOC_POOL(qscale_table, mv_table_size);
+    ALLOC_POOL(mb_type, mv_table_size * sizeof(uint32_t));
+
+    if (s->out_format == FMT_H263 || s->encoding ||
+        (s->avctx->export_side_data & AV_CODEC_EXPORT_DATA_MVS)) {
+        const int b8_array_size = s->b8_stride * mb_height * 2;
+        int mv_size        = 2 * (b8_array_size + 4) * sizeof(int16_t);
+        int ref_index_size = 4 * mb_array_size;
+
+        ALLOC_POOL(motion_val, mv_size);
+        ALLOC_POOL(ref_index, ref_index_size);
+    }
+#undef ALLOC_POOL
+    pools->alloc_mb_width  = s->mb_width;
+    pools->alloc_mb_height = mb_height;
+    pools->alloc_mb_stride = s->mb_stride;
+
     return !CONFIG_MPEGVIDEODEC || s->encoding ? 0 : ff_mpeg_er_init(s);
 }
 
 static void clear_context(MpegEncContext *s)
 {
+    memset(&s->buffer_pools, 0, sizeof(s->buffer_pools));
     memset(&s->next_picture, 0, sizeof(s->next_picture));
     memset(&s->last_picture, 0, sizeof(s->last_picture));
     memset(&s->current_picture, 0, sizeof(s->current_picture));
@@ -762,6 +798,7 @@  void ff_mpv_free_context_frame(MpegEncContext *s)
 {
     free_duplicate_contexts(s);
 
+    free_buffer_pools(&s->buffer_pools);
     av_freep(&s->p_field_mv_table_base);
     for (int i = 0; i < 2; i++)
         for (int j = 0; j < 2; j++)
diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
index 215df0fd5b..36ef6f5ff5 100644
--- a/libavcodec/mpegvideo.h
+++ b/libavcodec/mpegvideo.h
@@ -132,6 +132,8 @@  typedef struct MpegEncContext {
     Picture **input_picture;   ///< next pictures on display order for encoding
     Picture **reordered_input_picture; ///< pointer to the next pictures in coded order for encoding
 
+    BufferPoolContext buffer_pools;
+
     int64_t user_specified_pts; ///< last non-zero pts from AVFrame which was passed into avcodec_send_frame()
     /**
      * pts difference between the first and second input frame, used for
diff --git a/libavcodec/mpegvideo_dec.c b/libavcodec/mpegvideo_dec.c
index c1f49bce14..a4c7a0086a 100644
--- a/libavcodec/mpegvideo_dec.c
+++ b/libavcodec/mpegvideo_dec.c
@@ -115,12 +115,11 @@  int ff_mpeg_update_thread_context(AVCodecContext *dst,
 #define UPDATE_PICTURE(pic)\
 do {\
     ff_mpeg_unref_picture(&s->pic);\
-    if (s1->pic.f && s1->pic.f->buf[0])\
+    if (s1->pic.f && s1->pic.f->buf[0]) {\
         ret = ff_mpeg_ref_picture(&s->pic, &s1->pic);\
-    else\
-        ret = ff_update_picture_tables(&s->pic, &s1->pic);\
-    if (ret < 0)\
-        return ret;\
+        if (ret < 0)\
+            return ret;\
+    }\
 } while (0)
 
     UPDATE_PICTURE(current_picture);
@@ -194,10 +193,6 @@  int ff_mpv_common_frame_size_change(MpegEncContext *s)
 
     ff_mpv_free_context_frame(s);
 
-    if (s->picture)
-        for (int i = 0; i < MAX_PICTURE_COUNT; i++)
-            s->picture[i].needs_realloc = 1;
-
     s->last_picture_ptr         =
     s->next_picture_ptr         =
     s->current_picture_ptr      = NULL;
@@ -268,9 +263,12 @@  static int alloc_picture(MpegEncContext *s, Picture **picp, int reference)
     if (ret < 0)
         goto fail;
 
-    ret = ff_alloc_picture(s->avctx, pic, &s->me, &s->sc, 0, s->out_format,
-                           s->mb_stride, s->mb_width, s->mb_height, s->b8_stride,
-                           &s->linesize, &s->uvlinesize);
+    av_assert1(s->mb_width  == s->buffer_pools.alloc_mb_width);
+    av_assert1(s->mb_height == s->buffer_pools.alloc_mb_height ||
+               FFALIGN(s->mb_height, 2) == s->buffer_pools.alloc_mb_height);
+    av_assert1(s->mb_stride == s->buffer_pools.alloc_mb_stride);
+    ret = ff_alloc_picture(s->avctx, pic, &s->me, &s->sc, &s->buffer_pools,
+                           s->mb_height, &s->linesize, &s->uvlinesize);
     if (ret < 0)
         goto fail;
     *picp = pic;
@@ -388,8 +386,7 @@  int ff_mpv_frame_start(MpegEncContext *s, AVCodecContext *avctx)
     for (int i = 0; i < MAX_PICTURE_COUNT; i++) {
         if (!s->picture[i].reference ||
             (&s->picture[i] != s->last_picture_ptr &&
-             &s->picture[i] != s->next_picture_ptr &&
-             !s->picture[i].needs_realloc)) {
+             &s->picture[i] != s->next_picture_ptr)) {
             ff_mpeg_unref_picture(&s->picture[i]);
         }
     }
@@ -487,7 +484,7 @@  int ff_mpv_export_qp_table(const MpegEncContext *s, AVFrame *f, const Picture *p
 {
     AVVideoEncParams *par;
     int mult = (qp_type == FF_MPV_QSCALE_TYPE_MPEG1) ? 2 : 1;
-    unsigned int nb_mb = p->alloc_mb_height * p->alloc_mb_width;
+    unsigned int nb_mb = p->mb_height * p->mb_width;
 
     if (!(s->avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS))
         return 0;
@@ -496,10 +493,10 @@  int ff_mpv_export_qp_table(const MpegEncContext *s, AVFrame *f, const Picture *p
     if (!par)
         return AVERROR(ENOMEM);
 
-    for (unsigned y = 0; y < p->alloc_mb_height; y++)
-        for (unsigned x = 0; x < p->alloc_mb_width; x++) {
-            const unsigned int block_idx = y * p->alloc_mb_width + x;
-            const unsigned int     mb_xy = y * p->alloc_mb_stride + x;
+    for (unsigned y = 0; y < p->mb_height; y++)
+        for (unsigned x = 0; x < p->mb_width; x++) {
+            const unsigned int block_idx = y * p->mb_width + x;
+            const unsigned int     mb_xy = y * p->mb_stride + x;
             AVVideoBlockParams *const b = av_video_enc_params_block(par, block_idx);
 
             b->src_x = x * 16;
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
index 0e3255c0fb..da1d317ca0 100644
--- a/libavcodec/mpegvideo_enc.c
+++ b/libavcodec/mpegvideo_enc.c
@@ -1112,9 +1112,11 @@  static int alloc_picture(MpegEncContext *s, Picture *pic)
     pic->f->width  = avctx->width;
     pic->f->height = avctx->height;
 
-    return ff_alloc_picture(s->avctx, pic, &s->me, &s->sc, 1, s->out_format,
-                            s->mb_stride, s->mb_width, s->mb_height, s->b8_stride,
-                            &s->linesize, &s->uvlinesize);
+    av_assert1(s->mb_width  == s->buffer_pools.alloc_mb_width);
+    av_assert1(s->mb_height == s->buffer_pools.alloc_mb_height);
+    av_assert1(s->mb_stride == s->buffer_pools.alloc_mb_stride);
+    return ff_alloc_picture(s->avctx, pic, &s->me, &s->sc, &s->buffer_pools,
+                            s->mb_height, &s->linesize, &s->uvlinesize);
 }
 
 static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg)
@@ -1480,7 +1482,7 @@  static int select_input_picture(MpegEncContext *s)
                 s->next_picture_ptr &&
                 skip_check(s, s->input_picture[0], s->next_picture_ptr)) {
                 // FIXME check that the gop check above is +-1 correct
-                av_frame_unref(s->input_picture[0]->f);
+                ff_mpeg_unref_picture(s->input_picture[0]);
 
                 ff_vbv_update(s, 0);
 
@@ -1627,8 +1629,7 @@  no_output_pic:
             pic->display_picture_number = s->reordered_input_picture[0]->display_picture_number;
 
             /* mark us unused / free shared pic */
-            av_frame_unref(s->reordered_input_picture[0]->f);
-            s->reordered_input_picture[0]->shared = 0;
+            ff_mpeg_unref_picture(s->reordered_input_picture[0]);
 
             s->current_picture_ptr = pic;
         } else {