diff mbox

[FFmpeg-devel,v5] pngdec: expose gAMA and cHRM chunks as side/meta data

Message ID 20171109023223.19727-1-atomnuker@gmail.com
State Superseded
Headers show

Commit Message

Rostislav Pehlivanov Nov. 9, 2017, 2:32 a.m. UTC 
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavcodec/pngdec.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

James Almer Nov. 9, 2017, 2:44 a.m. UTC | #1 
On 11/8/2017 11:32 PM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavcodec/pngdec.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 0d6612ccca..0a9a248ee7 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -25,6 +25,7 @@
>  #include "libavutil/bprint.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/stereo3d.h"
> +#include "libavutil/mastering_display_metadata.h"
>  
>  #include "avcodec.h"
>  #include "bytestream.h"
> @@ -1165,7 +1166,7 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>      AVDictionary **metadatap = NULL;
>      uint32_t tag, length;
>      int decode_next_dat = 0;
> -    int ret;
> +    int i, ret;
>  
>      for (;;) {
>          length = bytestream2_get_bytes_left(&s->gb);
> @@ -1287,6 +1288,42 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>                  goto fail;
>              break;
>          }
> +        case MKTAG('c', 'H', 'R', 'M'): {
> +            AVMasteringDisplayMetadata *mdm = av_mastering_display_metadata_create_side_data(p);
> +            if (!mdm) {
> +                ret = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +
> +            mdm->white_point[0] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
> +            mdm->white_point[1] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
> +
> +            /* RGB Primaries */
> +            for (i = 0; i < 3; i++) {
> +                mdm->display_primaries[i][0] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
> +                mdm->display_primaries[i][1] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
> +            }
> +
> +            mdm->has_primaries = 1;
> +            bytestream2_skip(&s->gb, 4); /* crc */
> +            break;
> +        }
> +        case MKTAG('g', 'A', 'M', 'A'): {
> +            AVBPrint bp;
> +            char *gamma_str;
> +            int num = bytestream2_get_be32(&s->gb);
> +
> +            av_bprint_init(&bp, 0, -1);
> +            av_bprintf(&bp, "%i/%i", num, 100000);
> +            av_bprint_finalize(&bp, (char **)&gamma_str);

No need for a cast if gamma_str is already char*

> +            if (!gamma_str)
> +                return AVERROR(ENOMEM);
> +
> +            av_dict_set(&p->metadata, "gamma", gamma_str, 0);

Add the AV_DICT_DONT_STRDUP_VAL flag, otherwise gamma_str will leak.

> +
> +            bytestream2_skip(&s->gb, 4); /* crc */
> +            break;
> +        }
>          case MKTAG('I', 'E', 'N', 'D'):
>              if (!(s->pic_state & PNG_ALLIMAGE))
>                  av_log(avctx, AV_LOG_ERROR, "IEND without all image\n");
> 

LGTM aside from the above.
Rostislav Pehlivanov Nov. 9, 2017, 2:57 a.m. UTC | #2 
On 9 November 2017 at 02:44, James Almer <jamrial@gmail.com> wrote:

> On 11/8/2017 11:32 PM, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > ---
> >  libavcodec/pngdec.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > index 0d6612ccca..0a9a248ee7 100644
> > --- a/libavcodec/pngdec.c
> > +++ b/libavcodec/pngdec.c
> > @@ -25,6 +25,7 @@
> >  #include "libavutil/bprint.h"
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/stereo3d.h"
> > +#include "libavutil/mastering_display_metadata.h"
> >
> >  #include "avcodec.h"
> >  #include "bytestream.h"
> > @@ -1165,7 +1166,7 @@ static int decode_frame_common(AVCodecContext
> *avctx, PNGDecContext *s,
> >      AVDictionary **metadatap = NULL;
> >      uint32_t tag, length;
> >      int decode_next_dat = 0;
> > -    int ret;
> > +    int i, ret;
> >
> >      for (;;) {
> >          length = bytestream2_get_bytes_left(&s->gb);
> > @@ -1287,6 +1288,42 @@ static int decode_frame_common(AVCodecContext
> *avctx, PNGDecContext *s,
> >                  goto fail;
> >              break;
> >          }
> > +        case MKTAG('c', 'H', 'R', 'M'): {
> > +            AVMasteringDisplayMetadata *mdm =
> av_mastering_display_metadata_create_side_data(p);
> > +            if (!mdm) {
> > +                ret = AVERROR(ENOMEM);
> > +                goto fail;
> > +            }
> > +
> > +            mdm->white_point[0] = av_make_q(bytestream2_get_be32(&s->gb),
> 100000);
> > +            mdm->white_point[1] = av_make_q(bytestream2_get_be32(&s->gb),
> 100000);
> > +
> > +            /* RGB Primaries */
> > +            for (i = 0; i < 3; i++) {
> > +                mdm->display_primaries[i][0] =
> av_make_q(bytestream2_get_be32(&s->gb), 100000);
> > +                mdm->display_primaries[i][1] =
> av_make_q(bytestream2_get_be32(&s->gb), 100000);
> > +            }
> > +
> > +            mdm->has_primaries = 1;
> > +            bytestream2_skip(&s->gb, 4); /* crc */
> > +            break;
> > +        }
> > +        case MKTAG('g', 'A', 'M', 'A'): {
> > +            AVBPrint bp;
> > +            char *gamma_str;
> > +            int num = bytestream2_get_be32(&s->gb);
> > +
> > +            av_bprint_init(&bp, 0, -1);
> > +            av_bprintf(&bp, "%i/%i", num, 100000);
> > +            av_bprint_finalize(&bp, (char **)&gamma_str);
>
> No need for a cast if gamma_str is already char*
>
> > +            if (!gamma_str)
> > +                return AVERROR(ENOMEM);
> > +
> > +            av_dict_set(&p->metadata, "gamma", gamma_str, 0);
>
> Add the AV_DICT_DONT_STRDUP_VAL flag, otherwise gamma_str will leak.
>
> > +
> > +            bytestream2_skip(&s->gb, 4); /* crc */
> > +            break;
> > +        }
> >          case MKTAG('I', 'E', 'N', 'D'):
> >              if (!(s->pic_state & PNG_ALLIMAGE))
> >                  av_log(avctx, AV_LOG_ERROR, "IEND without all image\n");
> >
>
> LGTM aside from the above.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Pushed with your suggestions

Thanks
diff mbox

Patch

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 0d6612ccca..0a9a248ee7 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -25,6 +25,7 @@ 
 #include "libavutil/bprint.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/stereo3d.h"
+#include "libavutil/mastering_display_metadata.h"
 
 #include "avcodec.h"
 #include "bytestream.h"
@@ -1165,7 +1166,7 @@  static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
     AVDictionary **metadatap = NULL;
     uint32_t tag, length;
     int decode_next_dat = 0;
-    int ret;
+    int i, ret;
 
     for (;;) {
         length = bytestream2_get_bytes_left(&s->gb);
@@ -1287,6 +1288,42 @@  static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
                 goto fail;
             break;
         }
+        case MKTAG('c', 'H', 'R', 'M'): {
+            AVMasteringDisplayMetadata *mdm = av_mastering_display_metadata_create_side_data(p);
+            if (!mdm) {
+                ret = AVERROR(ENOMEM);
+                goto fail;
+            }
+
+            mdm->white_point[0] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
+            mdm->white_point[1] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
+
+            /* RGB Primaries */
+            for (i = 0; i < 3; i++) {
+                mdm->display_primaries[i][0] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
+                mdm->display_primaries[i][1] = av_make_q(bytestream2_get_be32(&s->gb), 100000);
+            }
+
+            mdm->has_primaries = 1;
+            bytestream2_skip(&s->gb, 4); /* crc */
+            break;
+        }
+        case MKTAG('g', 'A', 'M', 'A'): {
+            AVBPrint bp;
+            char *gamma_str;
+            int num = bytestream2_get_be32(&s->gb);
+
+            av_bprint_init(&bp, 0, -1);
+            av_bprintf(&bp, "%i/%i", num, 100000);
+            av_bprint_finalize(&bp, (char **)&gamma_str);
+            if (!gamma_str)
+                return AVERROR(ENOMEM);
+
+            av_dict_set(&p->metadata, "gamma", gamma_str, 0);
+
+            bytestream2_skip(&s->gb, 4); /* crc */
+            break;
+        }
         case MKTAG('I', 'E', 'N', 'D'):
             if (!(s->pic_state & PNG_ALLIMAGE))
                 av_log(avctx, AV_LOG_ERROR, "IEND without all image\n");