Opened 6 weeks ago

Closed 3 weeks ago

Last modified 3 weeks 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 5 weeks ago.
DNxHD 444 support for non-multiple-of-16 dimensions patch

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 weeks ago by richardpl

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

comment:2 Changed 6 weeks 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 6 weeks 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 5 weeks 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 5 weeks ago by devernay

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

comment:5 Changed 5 weeks 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 5 weeks ago by devernay

I sent it there, thanks.

comment:7 Changed 3 weeks 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 3 weeks ago by cehoyos (previous) (diff)
Note: See TracTickets for help on using tickets.