Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#6649 closed defect (fixed)

DNxHR 444 encoding crashes because of wrong macroblock size

Reported by: devernay Owned by:
Priority: important Component: avcodec
Version: git-master Keywords: DNxHD crash
Cc: onemda@gmail.com Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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;

Attachments (1)

patch-dnxhdenc.diff (2.2 KB) - added by devernay 10 months ago.
DNxHD 444 support for non-multiple-of-16 dimensions patch

Download all attachments as: .zip

Change History (8)

comment:1 Changed 10 months ago by richardpl

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

comment:2 Changed 10 months ago by 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

comment:3 Changed 10 months ago by 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

comment:4 Changed 10 months ago by 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.

Changed 10 months ago by devernay

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

comment:5 Changed 10 months ago by cehoyos

  • Keywords crash added; DNxHR removed
  • Priority changed from normal to important

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

comment:6 Changed 10 months ago by devernay

I sent it there, thanks.

comment:7 Changed 10 months ago by cehoyos

  • Resolution set to fixed
  • Status changed from new to closed

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

Last edited 10 months ago by cehoyos (previous) (diff)
Note: See TracTickets for help on using tickets.