diff mbox

[FFmpeg-devel] avcodec/dpx: Support for RGB 12-bit packed decoding

Message ID 133b22e5-2849-4cde-8da7-8332b4ae8a77@mediaarea.net
State Superseded
Headers show

Commit Message

Jerome Martinez Feb. 14, 2018, 12:46 p.m. UTC 
On 08/02/2018 11:28, Jerome Martinez wrote:
> Currently RGB and RGBA 12-bit are supported by DPX decoder only if 
> component values are padded (packing "Filled to 32-bit words, method A").
> This patch adds decoding of RGB and RGBA 12-bit with no padding 
> (packing "Packed into 32-bit words").
>
> As I have no file with line boundaries not aligned on 32-bit, I can 
> not have good tests about the stride computing (so code about non 
> aligned boundaries is theory) so I preferred to limit risks by 
> decoding only if line boundaries are aligned on 32-bit words:
> - 8 pixels for RGB (8 pixels x 3 components x 12 bits = 288 bits = 9 x 
> 32-bit words)
> - 2 pixels for RGBA (2 pixels x 4 components x 12 bits = 3 x 32-bit 
> words)
>
> I think Little Endian parsing is fine thanks to the generic code about 
> Big vs Little endian but I have no Little Endian test file so I also 
> limited the decoding to Big Endian files.
>
> Would be happy to check with cases I was not able to check if someone 
> provides files.
>
> I kept "Packing to 16bit required\n" message but interested in any 
> suggestion about a better wording due to exceptions.
>
> My test files:
> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGB_12_Packed_BE 
>
>
> Regression tests done on 12-bit content "Filled to 32-bit words, 
> method A" in:
> https://samples.ffmpeg.org/image-samples/dpx_samples.zip

Looks like the previous version of the patch is not included due to the 
debate about RGBA.
Please consider to include this modified patch, which has the RGBA part 
commented (I could remove the commented lines if you prefer).
From 28316686fed140279494d8c94e4f6bb5707e9ac0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net>
Date: Thu, 8 Feb 2018 09:22:08 +0100
Subject: [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding

Limited to widths multiple of 8 (RGB) due to lack of test files for such corner case

This partially fixes ticket #5639
---
 libavcodec/dpx.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 3 deletions(-)

Comments

Kieran O Leary April 10, 2018, 10:28 a.m. UTC | #1 
I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci
Resolve. Encoding to FFV1 and back again to DPX produced the same framemd5
values.

Does any more testing need to happen before this can be merged?

Best,

Kieran
Carl Eugen Hoyos April 10, 2018, 10:34 a.m. UTC | #2 
2018-04-10 12:28 GMT+02:00, Kieran O Leary <kieran.o.leary@gmail.com>:
> I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci
> Resolve.

Testing is good, apart from more brackets (and less comments) it would
be better if Jerome sends his public keys to Michael and pushes the patch.

> Encoding to FFV1 and back again to DPX produced the same
> framemd5 values.

(I believe we already discussed in public that testing FFmpeg's bugs
with FFmpeg makes limited sense.)

Carl Eugen
Paul B Mahol April 10, 2018, 10:41 a.m. UTC | #3 
On 4/10/18, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
> I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci
> Resolve. Encoding to FFV1 and back again to DPX produced the same framemd5
> values.
>
> Does any more testing need to happen before this can be merged?
>
> Best,
>
> Kieran
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Does this patch decodes to gbrp12 pixel format?
Carl Eugen Hoyos April 10, 2018, 11:02 a.m. UTC | #4 
2018-04-10 12:41 GMT+02:00, Paul B Mahol <onemda@gmail.com>:
> On 4/10/18, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
>> I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci
>> Resolve. Encoding to FFV1 and back again to DPX produced the same framemd5
>> values.
>>
>> Does any more testing need to happen before this can be merged?

> Does this patch decodes to gbrp12 pixel format?

Looking at this block of the patch, it appears so:
> +                    *dst[2]++ = read12in32(&buf, &rgbBuffer,
> +                                           &n_datum, endian);
> +                    *dst[0]++ = read12in32(&buf, &rgbBuffer,
> +                                           &n_datum, endian);
> +                    *dst[1]++ = read12in32(&buf, &rgbBuffer,
> +                                           &n_datum, endian);

(Above are "& 0xFFF")

Carl Eugen
Kieran O Leary April 10, 2018, 11:13 a.m. UTC | #5 
Hi

On Tue, Apr 10, 2018 at 11:34 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-04-10 12:28 GMT+02:00, Kieran O Leary <kieran.o.leary@gmail.com>:
>> I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci
>> Resolve.
>
> Testing is good, apart from more brackets (and less comments) it would
> be better if Jerome sends his public keys to Michael and pushes the patch.
>
>> Encoding to FFV1 and back again to DPX produced the same
>> framemd5 values.
>
> (I believe we already discussed in public that testing FFmpeg's bugs
> with FFmpeg makes limited sense.)
>

Yes, and at the risk of making a fool of myself, here's some slightly
different testing sing the graphicsmagick dpx encoder:
The following console outputs have 4 steps:

1. Use gm convert to decode 12-bit DPX to 12-bit DPX (padded to 16-bit DPX)
2. Use ffmpeg convert to decode 12-bit DPX to 12-bit DPX (padded to 16-bit DPX)
3. Use ffmpeg to decode the graphicsmagick file with the md5 muxer
4. Use ffmpeg to decode the ffmpeg file with the md5 muxer

The same md5 values are produced in steps 3 and 4- not sure what it
proves exactly, maybe that even if the ffmpeg decoder is buggy, at
least it decodes the files produced by Jerome's patch and the existing
GM decoder in the same way?

$ gm convert -verbose
6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx gm_dpx_to_dpx.dpx

6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx DPX 1600x1168+0+0
DirectClass 12-bit 8.0Mi 0.030u 0m:0.050000s

6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx=>gm_dpx_to_dpx.dpx
DPX 1600x1168+0+0 DirectClass 12-bit 10.7Mi 0.040u 0m:0.150000s
(11.9Mi pixels/s)



$ ./ffmpeg  -i 6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx
ffmpeg_dpx_to_dpx.dpx

ffmpeg version N-90642-gd64183e Copyright (c) 2000-2018 the FFmpeg developers

  built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

  configuration:

  libavutil      56. 13.100 / 56. 13.100

  libavcodec     58. 17.100 / 58. 17.100

  libavformat    58. 11.101 / 58. 11.101

  libavdevice    58.  2.100 / 58.  2.100

  libavfilter     7. 14.100 /  7. 14.100

  libswscale      5.  0.102 /  5.  0.102

  libswresample   3.  0.101 /  3.  0.101

[dpx_pipe @ 0x7f80db809000] Stream #0: not enough frames to estimate
rate; consider increasing probesize

Input #0, dpx_pipe, from '6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx':

  Duration: N/A, bitrate: N/A

    Stream #0:0: Video: dpx, gbrp12le, 1600x1168 [SAR 1:1 DAR 100:73],
24 tbr, 25 tbn, 24 tbc

Stream mapping:

  Stream #0:0 -> #0:0 (dpx (native) -> dpx (native))

Press [q] to stop, [?] for help

Output #0, image2, to 'ffmpeg_dpx_to_dpx.dpx':

  Metadata:

    encoder         : Lavf58.11.101

    Stream #0:0: Video: dpx, gbrp12le, 1600x1168 [SAR 1:1 DAR 100:73],
q=2-31, 200 kb/s, 24 fps, 24 tbn, 24 tbc

    Metadata:

      encoder         : Lavc58.17.100 dpx

frame=    1 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.04 bitrate=N/A
speed=0.286x

video:10952kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: unknown



$ ./ffmpeg -i ffmpeg_dpx_to_dpx.dpx -f md5 -

ffmpeg version N-90642-gd64183e Copyright (c) 2000-2018 the FFmpeg developers

  built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

  configuration:

  libavutil      56. 13.100 / 56. 13.100

  libavcodec     58. 17.100 / 58. 17.100

  libavformat    58. 11.101 / 58. 11.101

  libavdevice    58.  2.100 / 58.  2.100

  libavfilter     7. 14.100 /  7. 14.100

  libswscale      5.  0.102 /  5.  0.102

  libswresample   3.  0.101 /  3.  0.101

[dpx_pipe @ 0x7ff76b009000] Stream #0: not enough frames to estimate
rate; consider increasing probesize

Input #0, dpx_pipe, from 'ffmpeg_dpx_to_dpx.dpx':

  Duration: N/A, bitrate: N/A

    Stream #0:0: Video: dpx, gbrp12le, 1600x1168 [SAR 1:1 DAR 100:73],
25 tbr, 25 tbn, 25 tbc

Stream mapping:

  Stream #0:0 -> #0:0 (dpx (native) -> rawvideo (native))

Press [q] to stop, [?] for help

Output #0, md5, to 'pipe:':

  Metadata:

    encoder         : Lavf58.11.101

    Stream #0:0: Video: rawvideo (G3[0][12] / 0xC003347), gbrp12le,
1600x1168 [SAR 1:1 DAR 100:73], q=2-31, 1681920 kb/s, 25 fps, 25 tbn,
25 tbc

    Metadata:

      encoder         : Lavc58.17.100 rawvideo

MD5=aa43e3ec1c2a8e4499b3bc796a35185b

frame=    1 fps=0.0 q=-0.0 Lsize=       0kB time=00:00:00.04 bitrate=
 7.4kbits/s speed=1.25x

video:10950kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: unknown



$ ./ffmpeg -i gm_dpx_to_dpx.dpx -f md5 -

ffmpeg version N-90642-gd64183e Copyright (c) 2000-2018 the FFmpeg developers

  built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

  configuration:

  libavutil      56. 13.100 / 56. 13.100

  libavcodec     58. 17.100 / 58. 17.100

  libavformat    58. 11.101 / 58. 11.101

  libavdevice    58.  2.100 / 58.  2.100

  libavfilter     7. 14.100 /  7. 14.100

  libswscale      5.  0.102 /  5.  0.102

  libswresample   3.  0.101 /  3.  0.101

[dpx_pipe @ 0x7fe9d2809000] Stream #0: not enough frames to estimate
rate; consider increasing probesize

Input #0, dpx_pipe, from 'gm_dpx_to_dpx.dpx':

  Duration: N/A, bitrate: N/A

    Stream #0:0: Video: dpx, gbrp12le, 1600x1168 [SAR 1:1 DAR 100:73],
24 tbr, 25 tbn, 24 tbc

Stream mapping:

  Stream #0:0 -> #0:0 (dpx (native) -> rawvideo (native))

Press [q] to stop, [?] for help

Output #0, md5, to 'pipe:':

  Metadata:

    encoder         : Lavf58.11.101

    Stream #0:0: Video: rawvideo (G3[0][12] / 0xC003347), gbrp12le,
1600x1168 [SAR 1:1 DAR 100:73], q=2-31, 1614643 kb/s, 24 fps, 24 tbn,
24 tbc

    Metadata:

      encoder         : Lavc58.17.100 rawvideo

MD5=aa43e3ec1c2a8e4499b3bc796a35185b

frame=    1 fps=0.0 q=-0.0 Lsize=       0kB time=00:00:00.04 bitrate=
 7.1kbits/s speed=1.32x

video:10950kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: unknown
Kieran O Leary April 10, 2018, 1:24 p.m. UTC | #6 
Actually, here's another test that might be a bit more meaningful.
This is using graphicksmagic and the identify/signature command in
order to generate sha256 hashes for the image data only.

It is producing identical hashes for:
1. The original 12-bit DPX from Da Vinci Resolve (not packed to 16-bit)
2. The graphicsmagick 12-bit DPX to 12-bit DPX (padded to 16-bit DPX) file
3. The ffmpeg convert 12-bit DPX to 12-bit DPX (padded to 16-bit DPX)
which uses the ffmpeg decoder with Jerome's patch.

$ gm identify -verbose
6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx  |grep -i
signature

  Signature: 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c

$ gm identify -verbose gm_dpx_to_dpx.dpx  |grep -i signature

  Signature: 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c


$ gm identify -verbose ffmpeg_dpx_to_dpx.dpx  |grep -i signature

  Signature: 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c

Hopefully this is a bit better than just using ffmpeg?

Best,

Kieran.
Paul B Mahol April 10, 2018, 2:16 p.m. UTC | #7 
On 4/10/18, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
> Actually, here's another test that might be a bit more meaningful.
> This is using graphicksmagic and the identify/signature command in
> order to generate sha256 hashes for the image data only.
>
> It is producing identical hashes for:
> 1. The original 12-bit DPX from Da Vinci Resolve (not packed to 16-bit)
> 2. The graphicsmagick 12-bit DPX to 12-bit DPX (padded to 16-bit DPX) file
> 3. The ffmpeg convert 12-bit DPX to 12-bit DPX (padded to 16-bit DPX)
> which uses the ffmpeg decoder with Jerome's patch.
>
> $ gm identify -verbose
> 6e0edc3b-7d89-4710-8b9a-66c8c990f9dc_1_00094787.dpx  |grep -i
> signature
>
>   Signature:
> 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c
>
> $ gm identify -verbose gm_dpx_to_dpx.dpx  |grep -i signature
>
>   Signature:
> 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c
>
>
> $ gm identify -verbose ffmpeg_dpx_to_dpx.dpx  |grep -i signature
>
>   Signature:
> 3fb67059dca860c483b1deac912973b76d48fa9f0114d63e8116cff69d55bb4c
>
> Hopefully this is a bit better than just using ffmpeg?

No, you must not use tragic software ever.

Besides ffmpeg also can give you checksums for image only.
See framehash muxer, and it can give you sha512 hash.
Kieran O Leary April 10, 2018, 2:27 p.m. UTC | #8 
On Tue, Apr 10, 2018 at 3:16 PM, Paul B Mahol <onemda@gmail.com> wrote:
> On 4/10/18, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
>>
>> Hopefully this is a bit better than just using ffmpeg?
>
> No, you must not use tragic software ever.
>
> Besides ffmpeg also can give you checksums for image only.
> See framehash muxer, and it can give you sha512 hash.

Yes, I've used framehash but we use framemd5 internally in the Irish
Film Institute. I was just using another DPX decoder as Carl is
sceptical about using framemd5, or ffmpeg in general to determine
losslessness of files created by ffmpeg.
This was one of the only times I've ever used Image/GraphicsMagick to be honest.
diff mbox

Patch

diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
index 1aa2cbd1c8..7e2a6fb779 100644
--- a/libavcodec/dpx.c
+++ b/libavcodec/dpx.c
@@ -65,6 +65,38 @@  static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf,
     return *lbuf & 0x3FF;
 }
 
+static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf,
+                                  int * n_datum, int is_big)
+{
+    if (*n_datum)
+        (*n_datum)--;
+    else {
+        *lbuf = read32(ptr, is_big);
+        *n_datum = 7;
+    }
+
+    switch (*n_datum){
+    case 7: return *lbuf & 0xFFF;
+    case 6: return (*lbuf >> 12) & 0xFFF;
+    case 5: {
+            uint32_t c = *lbuf >> 24;
+            *lbuf = read32(ptr, is_big);
+            c |= *lbuf << 8;
+            return c & 0xFFF;
+            }
+    case 4: return (*lbuf >> 4) & 0xFFF;
+    case 3: return (*lbuf >> 16) & 0xFFF;
+    case 2: {
+            uint32_t c = *lbuf >> 28;
+            *lbuf = read32(ptr, is_big);
+            c |= *lbuf << 4;
+            return c & 0xFFF;
+            }
+    case 1: return (*lbuf >> 8) & 0xFFF;
+    default: return *lbuf >> 20;
+    }
+}
+
 static int decode_frame(AVCodecContext *avctx,
                         void *data,
                         int *got_frame,
@@ -201,10 +233,29 @@  static int decode_frame(AVCodecContext *avctx,
         break;
     case 12:
         if (!packing) {
-            av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n");
-            return -1;
+            int tested = 0;
+            if (endian && descriptor == 50 && (avctx->width%8) == 0) // Little endian and widths not a multiple of 8 need tests
+                tested = 1;
+            //if (endian && descriptor == 51 && (avctx->width%2) == 0) // Little endian and widths not a multiple of 2 need tests
+            //    tested = 1;
+            if (!tested) {
+                av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n");
+                return -1;
+            }
+        }
+        stride = avctx->width * elements;
+        if (packing)
+            stride *= 2;
+        else {
+            stride *= 3; // 12 bits are 1.5 byte so multiplied by 3 then divided by 2
+            if (stride % 8) {
+                // Align to 32-bit boundaries (not tested)
+                stride /= 8;
+                stride++;
+                stride *= 8;
+            }
+            stride /= 2;
         }
-        stride = 2 * avctx->width * elements;
         break;
     case 16:
         stride = 2 * avctx->width * elements;
@@ -349,6 +400,7 @@  static int decode_frame(AVCodecContext *avctx,
                                 (uint16_t*)ptr[2],
                                 (uint16_t*)ptr[3]};
             for (y = 0; y < avctx->width; y++) {
+                if (packing) {
                 if (elements >= 3)
                     *dst[2]++ = read16(&buf, endian) >> 4;
                 *dst[0] = read16(&buf, endian) >> 4;
@@ -357,6 +409,17 @@  static int decode_frame(AVCodecContext *avctx,
                     *dst[1]++ = read16(&buf, endian) >> 4;
                 if (elements == 4)
                     *dst[3]++ = read16(&buf, endian) >> 4;
+                } else {
+                    *dst[2]++ = read12in32(&buf, &rgbBuffer,
+                                           &n_datum, endian);
+                    *dst[0]++ = read12in32(&buf, &rgbBuffer,
+                                           &n_datum, endian);
+                    *dst[1]++ = read12in32(&buf, &rgbBuffer,
+                                           &n_datum, endian);
+                    if (elements == 4)
+                        *dst[3]++ = read12in32(&buf, &rgbBuffer,
+                                               &n_datum, endian);
+                }
             }
             for (i = 0; i < elements; i++)
                 ptr[i] += p->linesize[i];