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

Submitted by Jerome Martinez on Feb. 8, 2018, 10:28 a.m.

Details

Message ID 1d1e3171-771f-7e1d-0dd6-6bf04430ba5c@mediaarea.net
State New
Headers show

Commit Message

Jerome Martinez Feb. 8, 2018, 10:28 a.m.
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
From bf95371e3964e198e22dc545fc75706dedf9029b 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 and RGBA 12-bit packed decoding

Limited to widths multiple of 8 (RGB) and 2 (RGBA) due to lack of test files for such corner case
---
 libavcodec/dpx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 3 deletions(-)

Comments

Kieran O Leary Feb. 8, 2018, 3:23 p.m.
Hi Jerome,

Thank you so much for this patch.

I ran a quick test and ffmpeg and ffplay seems to decode these files
just fine. I noticed that taking a short 12-bit sequence (i didn't
even realise that they were big endian actually) from the Blackmagic
Cintel Scanner turned into FFV1 version 3 in Matroska with identical
framemd5s. I did get some past duration errors in the terminal though:

Here's a few frames that I used in this example. It seems to happen at
the 16th frame..
https://www.dropbox.com/sh/imaasud6yora8me/AAAXXGIQ9w6fFfvx9qigizyVa?dl=0https://www.dropbox.com/sh/imaasud6yora8me/AAAXXGIQ9w6fFfvx9qigizyVa?dl=0

./ffmpeg -start_number 00086400 -i
first_frames/99075c82-2770-4020-a289-d191dab0b852_1_%08d.dpx -c:v ffv1
-level 3 out.mkv

ffmpeg version N-89977-gddd851f Copyright (c) 2000-2018 the FFmpeg developers

  built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

  configuration:

  libavutil      56.  7.100 / 56.  7.100

  libavcodec     58. 10.100 / 58. 10.100

  libavformat    58.  9.100 / 58.  9.100

  libavdevice    58.  1.100 / 58.  1.100

  libavfilter     7. 11.101 /  7. 11.101

  libswscale      5.  0.101 /  5.  0.101

  libswresample   3.  0.101 /  3.  0.101

Input #0, image2, from
'first_frames/99075c82-2770-4020-a289-d191dab0b852_1_%08d.dpx':

  Duration: 00:00:00.92, start: 0.000000, bitrate: N/A

    Stream #0:0: Video: dpx, gbrp12le, 2160x1702 [SAR 1:1 DAR
1080:851], 24 tbr, 25 tbn, 24 tbc

Stream mapping:

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

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

[ffv1 @ 0x7fd89c013800] bits_per_raw_sample > 8, forcing range coder

Output #0, matroska, to 'out.mkv':

  Metadata:

    encoder         : Lavf58.9.100

    Stream #0:0: Video: ffv1 (FFV1 / 0x31564646), gbrp12le, 2160x1702
[SAR 1:1 DAR 1080:851], q=2-31, 200 kb/s, 24 fps, 1k tbn, 24 tbc

    Metadata:

      encoder         : Lavc58.10.100 ffv1

frame=    2 fps=0.0 q=-0.0 size=   11653kB time=00:00:00.04
bitrate=2220121.7kbiframe=    4 fps=3.4 q=-0.0 size=   34930kB
time=00:00:00.12 bitrate=2270993.5kbiframe=    6 fps=3.4 q=-0.0 size=
 58259kB time=00:00:00.20 bitrate=2283544.4kbiframe=    8 fps=3.3
q=-0.0 size=   81588kB time=00:00:00.29 bitrate=2281121.4kbiframe=
10 fps=3.3 q=-0.0 size=  104852kB
time=00:00:00.37bitrate=2284434.3kbiframe=   12 fps=3.3 q=-0.0 size=
128130kB time=00:00:00.45 bitrate=2286805.8kbiframe=   14 fps=3.3
q=-0.0 size=  151452kB time=00:00:00.54 bitrate=2284891.4kbiframe=
16 fps=3.3 q=-0.0 size=  174741kB time=00:00:00.62
bitrate=2286710.7kbiPast duration 0.639992 too large

Past duration 0.679985 too large

frame=   18 fps=3.2 q=-0.0 size=  198043kB time=00:00:00.70
bitrate=2288247.4kbiPast duration 0.719994 too large

Past duration 0.759987 too large

frame=   20 fps=3.2 q=-0.0 size=  221335kB time=00:00:00.79
bitrate=2286478.6kbiPast duration 0.799995 too large

Past duration 0.839989 too large

frame=   22 fps=3.2 q=-0.0 size=  244648kB time=00:00:00.87
bitrate=2287850.0kbiPast duration 0.879997 too large

frame=   23 fps=3.1 q=-0.0 size=  256309kB time=00:00:00.91
bitrate=2287235.2kbiframe=   23 fps=3.1 q=-0.0 Lsize=  267948kB
time=00:00:00.91 bitrate=2391103.3kbits/s speed=0.123x

video:267947kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: 0.000610%

Best,

Kieran O'Leary
IFI Irish Film Archive
Carl Eugen Hoyos Feb. 9, 2018, 1:19 a.m.
2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
> 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").

I suggest we wait with this patch until we verified if RGBA dpx decoding
is broken (I suspect so, I believe you have a sample to verify).

As-is, we wouldn't do anybody a favour.

Thank you, Carl Eugen
Jerome Martinez Feb. 9, 2018, 6:38 a.m.
On 09/02/2018 02:19, Carl Eugen Hoyos wrote:
> 2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
>> 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").
> I suggest we wait with this patch until we verified if RGBA dpx decoding
> is broken (I suspect so, I believe you have a sample to verify).

Sorry, I see a line was missing in my first post, the one with the link 
to the RGBA test file.
Test file for RGBA 12-bit packed:
https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE

Regression test done on 
checkerboard_1080p_nuke_bigendian_12bit_alpha.dpx and 
checkerboard_1080p_nuke_littleendian_12bit_alpha.dpx in 
https://samples.ffmpeg.org/image-samples/dpx_samples.zip


>
> As-is, we wouldn't do anybody a favour.
>
> Thank you, Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Carl Eugen Hoyos Feb. 9, 2018, 10:15 a.m.
2018-02-09 7:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
> On 09/02/2018 02:19, Carl Eugen Hoyos wrote:
>>
>> 2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
>>>
>>> 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").
>>
>> I suggest we wait with this patch until we verified if RGBA dpx decoding
>> is broken (I suspect so, I believe you have a sample to verify).
>
>
> Sorry, I see a line was missing in my first post, the one with the link to
> the RGBA test file.
> Test file for RGBA 12-bit packed:
> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE

Is this a sample from a four-channel film scanner (with infrared channel)?

Thank you, Carl Eugen
Carl Eugen Hoyos Feb. 9, 2018, 10:32 a.m.
2018-02-09 11:15 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 2018-02-09 7:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:

>> Sorry, I see a line was missing in my first post, the one with the link to
>> the RGBA test file.
>> Test file for RGBA 12-bit packed:
>> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE
>
> Is this a sample from a four-channel film scanner (with infrared channel)?

No, it is not, please mention ticket #5639 in the commit message.

Carl Eugen
Jerome Martinez Feb. 9, 2018, 10:34 a.m.
On 09/02/2018 11:15, Carl Eugen Hoyos wrote:
>
>> Sorry, I see a line was missing in my first post, the one with the link to
>> the RGBA test file.
>> Test file for RGBA 12-bit packed:
>> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE
> Is this a sample from a four-channel film scanner (with infrared channel)?

I think the question is out of topic for this patch proposal, as this 
question is global for all flavors of DPX (also the ones already 
supported by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled 
with Method A, here I just add the support for packed content, without 
adding anything else about alpha support in FFmpeg, this patch changes 
nothing about alpha support in FFmpeg and/or DPX) and the need for 
validating such patch is about sample files for RGBA 12-bit packed (RGBA 
12-bit filled with method A being already available) and not for the 
content itself.

If it is blocking for the patch inclusion, I could remove the line 
permitting RGBA Packed decoding.
Carl Eugen Hoyos Feb. 9, 2018, 10:39 a.m.
2018-02-09 11:34 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
> On 09/02/2018 11:15, Carl Eugen Hoyos wrote:
>>
>>
>>> Sorry, I see a line was missing in my first post, the one with the link
>>> to
>>> the RGBA test file.
>>> Test file for RGBA 12-bit packed:
>>>
>>> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE
>>
>> Is this a sample from a four-channel film scanner (with infrared channel)?
>
>
> I think the question is out of topic for this patch proposal, as this
> question is global for all flavors of DPX (also the ones already supported
> by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A,
> here I just add the support for packed content, without adding anything else
> about alpha support in FFmpeg, this patch changes nothing about alpha
> support in FFmpeg and/or DPX) and the need for validating such patch is
> about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A
> being already available) and not for the content itself.

Sorry, I thought you had access to such a sample and we would
just have to test it, no?

One more minor issue:
> +                }
> +                else {

This is getting very common now (do you know why?), please fix it.

Thank you, Carl Eugen
Jerome Martinez Feb. 12, 2018, 7:47 p.m.
On 09/02/2018 11:39, Carl Eugen Hoyos wrote:
>
>> I think the question is out of topic for this patch proposal, as this
>> question is global for all flavors of DPX (also the ones already supported
>> by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A,
>> here I just add the support for packed content, without adding anything else
>> about alpha support in FFmpeg, this patch changes nothing about alpha
>> support in FFmpeg and/or DPX) and the need for validating such patch is
>> about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A
>> being already available) and not for the content itself.
> Sorry, I thought you had access to such a sample and we would
> just have to test it, no?

https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
is indicated by the person who provided it as with DPX alpha channel 
used for actually storing infrared
Carl Eugen Hoyos Feb. 12, 2018, 9:37 p.m.
2018-02-12 20:47 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:

> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
> is indicated by the person who provided it as with DPX alpha channel used
> for actually storing infrared

Thank you!

This sample appears to confirm that GraphicsMagick is right and FFmpeg
is buggy. To avoid creating more incorrect (ffv1) encodes, I (strongly)
suggest to first fix this issue and commit your patch to support more
bit-depths but if you disagree, please feel free to commit!

There is also this sample though:
http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/

The only solution I can think of is to change the semantics of the fourth
dpx layer by default and to add an option (to the decoder) that allows using
the current semantics that defaults to "auto" reading the encoder value.

Carl Eugen
Jerome Martinez Feb. 12, 2018, 10:38 p.m.
On 12/02/2018 22:37, Carl Eugen Hoyos wrote:
> 2018-02-12 20:47 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
>
>> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
>> is indicated by the person who provided it as with DPX alpha channel used
>> for actually storing infrared
> Thank you!
>
> This sample appears to confirm that GraphicsMagick is right and FFmpeg
> is buggy. To avoid creating more incorrect (ffv1) encodes, I (strongly)
> suggest to first fix this issue and commit your patch to support more
> bit-depths but if you disagree, please feel free to commit!

Sorry but I don't get it: the patch in this thread is totally separated 
from the alpha channel meaning issue. What should I "commit" about which 
"issue"? The only issues I have for the moment are that 12-bit padded 
DPX is supported by FFmpeg but not 12-bit packed DPX (the patch solves 
it), and that FFV1 support is with e.g. 12-bit YUVA but not 12-bit RGBA 
(I'll send a patch tomorrow for that, separated issue).

I am not against continuing the discussion about the alpha channel 
meaning issue in a global manner, but I wish it is not blocking for this 
patch inclusion (I already sent the modified patch in order to fix the 
issues you indicated), as this patch does not create something new 
compared to what already exists (RGBA 8-bit, 16-bit, 10-bit padded, 
12-bit padded, are already parsed by FFmpeg DPX parser) and the patch 
may be useful for people using "correctly" the alpha channel as they do 
for the other flavors of DPX, as well for people using RGB 12-bit packed.

Let me know if I should split the patch in 2 (RGB part, RGBA part), so 
at least the RGB one could be included, as it is not related with any 
"alpha channel" stuff.

> There is also this sample though:
> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/
>
> The only solution I can think of is to change the semantics of the fourth
> dpx layer by default and to add an option (to the decoder) that allows using
> the current semantics that defaults to "auto" reading the encoder value.

IMO having the default everywhere as currently set is not "buggy", but a 
global (not limited to DPX) option could be interesting for setting the 
semantics of the channel flagged as "alpha", because people may already 
use DPX or FFV1 (FFV1 supports RGBA 8-bit or YUVA 16-bit for a long 
time, nothing new) or any other format with alpha channel not having the 
"right" semantics (and whatever is the format, it is impossible to know 
how it is used)
Carl Eugen Hoyos Feb. 12, 2018, 10:48 p.m.
2018-02-12 23:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>:
> On 12/02/2018 22:37, Carl Eugen Hoyos wrote:

>> The only solution I can think of is to change the semantics of the fourth
>> dpx layer by default and to add an option (to the decoder) that allows
>> using the current semantics that defaults to "auto" reading the encoder
>> value.
>
> IMO having the default everywhere as currently set is not "buggy", but a
> global (not limited to DPX) option could be interesting

I don't think this is a "global" issue, afaict, this is 100% dpx-specific.

I also don't think a global flag (or similar) that indicates that alpha
is actually infrared would me sense: GraphicsMagick (that does
not show the issue that FFmpeg has) does not have such a flag
and also supports a number of image formats that support alpha.

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
index 1aa2cbd1c8..aaacd243c9 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,18 @@  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];