diff mbox

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

Message ID 6eaaed88-607c-abc4-ba73-5414f931f7dc@mediaarea.net
State New
Headers show

Commit Message

Jerome Martinez Feb. 9, 2018, 11:10 a.m. UTC 
On 09/02/2018 11:32, Carl Eugen Hoyos wrote:
> [...] please mention ticket #5639 in the commit message.

I should definitely have indicated this source instead of my copy, my 
mistake.
Commit message extended with it.
FYI I am running tests on several DPX flavors (to FFV1 and back to DPX, 
checking that framemd5 is same in order to be sure the lossless 
compression is really lossless), including the ones in this ticket, I'll 
post a message in this ticket when I finish and all files are well 
supported in FFmpeg.

On 09/02/2018 11:39, Carl Eugen Hoyos wrote:
>
> Sorry, I thought you had access to such a sample and we would
> just have to test it, no?

I use to stay on a technical side, if the DPX header says alpha I map to 
alpha (no way to differentiate from an illegitimate content, technically 
speaking), whatever are the hacks used by users (my goal is to store 
content with FFV1 in order to save disk space then convert back to DPX 
when needed, so such hack are the user problem and are not visible by 
FFmpeg). Sometimes I have contact with the source provider, sometimes 
not. I think some of the files I get are hacked with infrared instead of 
alpha, if you are interested in a sample file with infrared-hacked 
content for sure I'll ask if it is the case for some files I have. But 
not blocking for the patch IMO (especially because the files I am 
thinking to are not 12-bit)

>
> One more minor issue:
>> +                }
>> +                else {
> This is getting very common now (do you know why?),

Different projects, different C style standards, so sometimes some 
mistakes when people work on several projects.
(here, it was due to automatic edition from a code editor having a 
different standard by default, and I missed it when I reviewed the style)

>   please fix it.

Fixed, updated patch attached.
From 986379350bc788701e39e2d6f9fce55b61b5fbb8 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

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

Comments

Kieran O Leary Feb. 9, 2018, 11:20 a.m. UTC | #1 
Hi both,

I'm just wondering if the past duration errors in my earlier email mean
anything,or should they be ignored?
Jerome Martinez Feb. 9, 2018, 11:27 a.m. UTC | #2 
On 09/02/2018 12:20, Kieran O Leary wrote:
> Hi both,
>
> I'm just wondering if the past duration errors in my earlier email mean
> anything,or should they be ignored?

They are independent from the patch, e.g.:
https://stackoverflow.com/questions/30782771/what-does-past-duration-x-xxx-too-large-mean
diff mbox

Patch

diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
index 1aa2cbd1c8..7535de6c23 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];