DNxHR 444 encoding crashes because of wrong macroblock size

[Frederic Devernay]

Summary of the bug:
I get a crash when encoding DNxHR 444 with an image height which is not a multiple of 16 (eg 1920x1080)
How to reproduce:
This does not seem to occur using the command-line ffmpeg utility, but using the ffmpeg API.

The bug seems to be at line:
​https://github.com/FFmpeg/FFmpeg/commit/f078bc4c5e6675a93166a7e5b23feb5b04ac9320#diff-f75ae6c1ad5f3f6c90ed8741497d2ce1R754

It supposes that the macroblock size for 10-bit images is 8 instead of 16, which looks wrong, given the fact that the macroblock size is always 16 according to ​https://github.com/FFmpeg/FFmpeg/blob/f078bc4c5e6675a93166a7e5b23feb5b04ac9320/libavcodec/dnxhdenc.c#L475
the following two-line patch fixes the crash/bug, but this should be reviewed by Paul B Mahol, who commited ​https://github.com/FFmpeg/FFmpeg/commit/f078bc4c5e6675a93166a7e5b23feb5b04ac9320 :

--- ffmpeg-3.3.3/libavcodec/dnxhdenc.c	2017-07-29 19:49:30.000000000 +0200
+++ ffmpeg-3.3.3.new/libavcodec/dnxhdenc.c	2017-09-11 19:41:13.000000000 +0200
@@ -751,10 +751,10 @@
         ptr_y = &ctx->edge_buf_y[0];
         ptr_u = &ctx->edge_buf_uv[0][0];
         ptr_v = &ctx->edge_buf_uv[1][0];
-    } else if (ctx->bit_depth == 10 && vdsp->emulated_edge_mc && ((mb_x << 3) + 8 > ctx->m.avctx->width ||
-                                                                  (mb_y << 3) + 8 > ctx->m.avctx->height)) {
-        int y_w = ctx->m.avctx->width  - (mb_x << 3);
-        int y_h = ctx->m.avctx->height - (mb_y << 3);
+    } else if (ctx->bit_depth == 10 && vdsp->emulated_edge_mc && ((mb_x << 4) + 16 > ctx->m.avctx->width ||
+                                                                  (mb_y << 4) + 16 > ctx->m.avctx->height)) {
+        int y_w = ctx->m.avctx->width  - (mb_x << 4);
+        int y_h = ctx->m.avctx->height - (mb_y << 4);
         int uv_w = ctx->is_444 ? y_w : (y_w + 1) / 2;
         int uv_h = y_h;
         linesize = 16;

[Elon Musk]

This creates invalid files. Can you get backtrace of crash you are experiencing?

[Frederic Devernay]

I agree, the last lines of the image are corrupted.

Here's the crash. It happens on the last macroblock line (mb_x=0 mb_y= 67), which goes beyond the limits of the 1920x1080 input image by 8 pixels (68*16=1088).

However, the condition of the test was not triggered, because mb_y << 3 + 8 = 544, which is half of ctx->m.avctx->height=1088.

My fix of the test itself is probably right, but there are certainly more errors inside the if() section after the test.

* thread #17, name = 'Parallel render thread (0x600001042590)', stop reason = EXC_BAD_ACCESS (code=1, address=0x1b6b78700)
  * frame #0: 0x000000019ab09a8b libavcodec.57.dylib`get_pixels_16_c(block=0x00000001a027c080, pixels="", stride=3840) at pixblockdsp.c:31
    frame #1: 0x000000019a5ffe1a libavcodec.57.dylib`dnxhd_calc_bits_thread [inlined] dnxhd_get_blocks(ctx=0x00000001a0279000, mb_x=0, mb_y=67) at dnxhdenc.c:832
    frame #2: 0x000000019a5ff69f libavcodec.57.dylib`dnxhd_calc_bits_thread(avctx=0x0000000108311200, arg=0x0000000000000000, jobnr=67, threadnr=0) at dnxhdenc.c:871
    frame #3: 0x000000019ac6d312 libavcodec.57.dylib`avcodec_default_execute2(c=0x0000000108311200, func=(libavcodec.57.dylib`dnxhd_calc_bits_thread at dnxhdenc.c:853), arg=0x0000000000000000, ret=0x0000000000000000, count=68) at utils.c:1028
    frame #4: 0x000000019a6007d1 libavcodec.57.dylib`dnxhd_find_qscale(ctx=0x00000001a0279000) at dnxhdenc.c:1126
    frame #5: 0x000000019a5fd270 libavcodec.57.dylib`dnxhd_encode_fast(avctx=0x0000000108311200, ctx=0x00000001a0279000) at dnxhdenc.c:1227
    frame #6: 0x000000019a5fae1d libavcodec.57.dylib`dnxhd_encode_picture(avctx=0x0000000108311200, pkt=0x000070000b83f588, frame=0x000000019ffb7900, got_packet=0x000070000b83f584) at dnxhdenc.c:1312
    frame #7: 0x000000019ac70d5e libavcodec.57.dylib`avcodec_encode_video2(avctx=0x0000000108311200, avpkt=0x000070000b83f588, frame=0x000000019ffb7900, got_packet_ptr=0x000070000b83f584) at utils.c:2008

[Frederic Devernay]

• the size of edge_buf_y, edge_buf_uv[0] and edge_buf_uv[1] (defined in dnxhdenc.h) should be 512 bytes, not 256, to cope for 16-bit data
• when the bit depth is 10, I think linesize should be 32 and uvlinesize should be 16 + 16*ctx->is_444

[Frederic Devernay]

ok I got a full fix.
That part of the code had several bugs, because it was basically dead code, since the condition was wrong.
Overall, this code looks very fragile:

• hardcoded numbers such as 16 (shouldn't the variable bs be used instead) and 1080 (why?) at several places
• the test ctx->bit_depth != 10 in dnxhd_get_blocks should really be ctx->bit_depth == 8
• the test ctx->bit_depth == 10 in dnxhd_get_blocks should really be ctx->bit_depth > 8 (remember that DNxHR 444 should really be 12-bit deep, so I hope we will get that into ffmpeg someday)
• hardcoded edge macroblock buffer sizes of 256 bytes (edge_buf_y and edge_buf_uv in dnxhdenc.h)

but at least I got DNxHR 444 working using the API!

I think the ffmpeg command-line tool worked by chance, because there is accessible memory after the frame to be encoded, so reading this area didn't make ffmpeg crash.

[Frederic Devernay]

DNxHD 444 support for non-multiple-of-16 dimensions patch

[Carl Eugen Hoyos]

Please send your patch - made with git format-patch - to the development mailing where it can be reviewed.

[Frederic Devernay]

I sent it there, thanks.

[Carl Eugen Hoyos]

Applied as eec67f25224a48047da57be18b610b11b0fd0bfe - thank you for the report and the fix!