[FFmpeg-devel,2/2] avcodec/dpx: improve decoding support for 10 bit depth

Submitted by Paul B Mahol on Dec. 6, 2018, 10:56 a.m.

Details

Message ID 20181206105640.12774-2-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Dec. 6, 2018, 10:56 a.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/dpx.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Comments

Carl Eugen Hoyos Dec. 6, 2018, 6:27 p.m.
2018-12-06 11:56 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/dpx.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
> index 0297287938..84894abda5 100644
> --- a/libavcodec/dpx.c
> +++ b/libavcodec/dpx.c
> @@ -50,6 +50,24 @@ static unsigned int read32(const uint8_t **ptr, int
> is_big)
>      return temp;
>  }
>
> +static uint16_t read10in32_gray(const uint8_t **ptr, uint32_t *lbuf,
> +                                int *n_datum, int is_big, int shift)
> +{
> +    uint16_t temp;
> +
> +    if (*n_datum)
> +        (*n_datum)--;
> +    else {
> +        *lbuf = read32(ptr, is_big);
> +        *n_datum = 2;
> +    }
> +

> +    temp = *lbuf >> shift & 0x3FF;
> +    *lbuf = *lbuf >> 10;
> +
> +    return temp;

Isn't my code simpler?
One variable less, same number of operations iirc.


> +}
> +
>  static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf,
>                                    int * n_datum, int is_big, int shift)
>  {
> @@ -107,6 +125,7 @@ static int decode_frame(AVCodecContext *avctx,
>      AVFrame *const p = data;
>      uint8_t *ptr[AV_NUM_DATA_POINTERS];
>      uint32_t header_version, version = 0;
> +    const uint8_t *creator;
>
>      unsigned int offset;
>      int magic_num, endian;
> @@ -151,6 +170,9 @@ static int decode_frame(AVCodecContext *avctx,
>          av_log(avctx, AV_LOG_WARNING, "Unknown header format version
> %s.\n",
>                 av_fourcc2str(header_version));
>

> +    buf = avpkt->data + 160;
> +    creator = buf;

I wonder why you split the version but not the creator
and why the metadata isn't set.

> +
>      // Check encryption
>      buf = avpkt->data + 660;
>      ret = read32(&buf, endian);
> @@ -320,6 +342,7 @@ static int decode_frame(AVCodecContext *avctx,
>      case 51121:
>          avctx->pix_fmt = AV_PIX_FMT_GBRAP12;

>          break;
> +    case 6100:

Looks unrelated.

>      case 6101:
>          avctx->pix_fmt = AV_PIX_FMT_GRAY10;
>          break;
> @@ -373,13 +396,17 @@ static int decode_frame(AVCodecContext *avctx,
>                                  (uint16_t*)ptr[1],
>                                  (uint16_t*)ptr[2],
>                                  (uint16_t*)ptr[3]};
> -            int shift = packing == 1 ? 22 : 20;
> +            int shift = elements > 1 ? packing == 1 ? 22 : 20 : packing ==
> 1 ? 2 : 0;

Wouldn't it be simpler to add "20" in read10in32()?

>              for (y = 0; y < avctx->width; y++) {
>                  if (elements >= 3)
>                      *dst[2]++ = read10in32(&buf, &rgbBuffer,
>                                             &n_datum, endian, shift);
> -                *dst[0]++ = read10in32(&buf, &rgbBuffer,
> -                                       &n_datum, endian, shift);
> +                if (elements == 1)
> +                    *dst[0]++ = read10in32_gray(&buf, &rgbBuffer,
> +                                                &n_datum, endian, shift);
> +                else
> +                    *dst[0]++ = read10in32(&buf, &rgbBuffer,
> +                                           &n_datum, endian, shift);
>                  if (elements >= 2)
>                      *dst[1]++ = read10in32(&buf, &rgbBuffer,
>                                             &n_datum, endian, shift);
> @@ -388,7 +415,8 @@ static int decode_frame(AVCodecContext *avctx,
>                      read10in32(&buf, &rgbBuffer,
>                                 &n_datum, endian, shift);
>              }

> -            n_datum = 0;
> +            if (version != 2 || !memcmp(creator, "GraphicsMagick", 14))

This looks wrong to me:
Where in the specification does it say that this depends on the version?

> +                n_datum = 0;
>              for (i = 0; i < elements; i++)
>                  ptr[i] += p->linesize[i];
>          }

Thank you, Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
index 0297287938..84894abda5 100644
--- a/libavcodec/dpx.c
+++ b/libavcodec/dpx.c
@@ -50,6 +50,24 @@  static unsigned int read32(const uint8_t **ptr, int is_big)
     return temp;
 }
 
+static uint16_t read10in32_gray(const uint8_t **ptr, uint32_t *lbuf,
+                                int *n_datum, int is_big, int shift)
+{
+    uint16_t temp;
+
+    if (*n_datum)
+        (*n_datum)--;
+    else {
+        *lbuf = read32(ptr, is_big);
+        *n_datum = 2;
+    }
+
+    temp = *lbuf >> shift & 0x3FF;
+    *lbuf = *lbuf >> 10;
+
+    return temp;
+}
+
 static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf,
                                   int * n_datum, int is_big, int shift)
 {
@@ -107,6 +125,7 @@  static int decode_frame(AVCodecContext *avctx,
     AVFrame *const p = data;
     uint8_t *ptr[AV_NUM_DATA_POINTERS];
     uint32_t header_version, version = 0;
+    const uint8_t *creator;
 
     unsigned int offset;
     int magic_num, endian;
@@ -151,6 +170,9 @@  static int decode_frame(AVCodecContext *avctx,
         av_log(avctx, AV_LOG_WARNING, "Unknown header format version %s.\n",
                av_fourcc2str(header_version));
 
+    buf = avpkt->data + 160;
+    creator = buf;
+
     // Check encryption
     buf = avpkt->data + 660;
     ret = read32(&buf, endian);
@@ -320,6 +342,7 @@  static int decode_frame(AVCodecContext *avctx,
     case 51121:
         avctx->pix_fmt = AV_PIX_FMT_GBRAP12;
         break;
+    case 6100:
     case 6101:
         avctx->pix_fmt = AV_PIX_FMT_GRAY10;
         break;
@@ -373,13 +396,17 @@  static int decode_frame(AVCodecContext *avctx,
                                 (uint16_t*)ptr[1],
                                 (uint16_t*)ptr[2],
                                 (uint16_t*)ptr[3]};
-            int shift = packing == 1 ? 22 : 20;
+            int shift = elements > 1 ? packing == 1 ? 22 : 20 : packing == 1 ? 2 : 0;
             for (y = 0; y < avctx->width; y++) {
                 if (elements >= 3)
                     *dst[2]++ = read10in32(&buf, &rgbBuffer,
                                            &n_datum, endian, shift);
-                *dst[0]++ = read10in32(&buf, &rgbBuffer,
-                                       &n_datum, endian, shift);
+                if (elements == 1)
+                    *dst[0]++ = read10in32_gray(&buf, &rgbBuffer,
+                                                &n_datum, endian, shift);
+                else
+                    *dst[0]++ = read10in32(&buf, &rgbBuffer,
+                                           &n_datum, endian, shift);
                 if (elements >= 2)
                     *dst[1]++ = read10in32(&buf, &rgbBuffer,
                                            &n_datum, endian, shift);
@@ -388,7 +415,8 @@  static int decode_frame(AVCodecContext *avctx,
                     read10in32(&buf, &rgbBuffer,
                                &n_datum, endian, shift);
             }
-            n_datum = 0;
+            if (version != 2 || !memcmp(creator, "GraphicsMagick", 14))
+                n_datum = 0;
             for (i = 0; i < elements; i++)
                 ptr[i] += p->linesize[i];
         }