[FFmpeg-devel] [PATCH] Update HDR10+ metadata structure.

[Mohammad Izadi]

Please check my responses inline:


On Wed, Feb 5, 2020 at 5:48 AM Moritz Barsnick <barsnick at gmx.net> wrote:

> Hi Mohammad,
>
> On Tue, Feb 04, 2020 at 18:44:00 -0800, Mohammad Izadi wrote:
> > --- a/libavutil/hdr_dynamic_metadata.c
> > +++ b/libavutil/hdr_dynamic_metadata.c
> > @@ -1,4 +1,4 @@
> > -/**
> > + /**
>
> Please review your git diff and your submitted patches carefully. You
> need to avoid this accidental change.
>
reverted the change.

>
> > +    if(!data)
>
> ffmpeg style: "if ("
>
Fixed.

>
> > +    if(ret < 0)
> > +        return AVERROR_INVALIDDATA;
>
> Ditto.
>
> Fixed.

> > +    if (get_bits_left(&gb) < 2)
> > +        return AVERROR_INVALIDDATA;
> > +    s->num_windows = get_bits(&gb, 2);
> > +
> > +    if (s->num_windows < 1 || s->num_windows > 3) {
> > +        return AVERROR_INVALIDDATA;
> > +    }
>
> Above, you skip the brackets for the one-liner return. Here, you use
> them. That's inconsistent.
>
> Fixed.

> > --- a/libavutil/hdr_dynamic_metadata.h
> > +++ b/libavutil/hdr_dynamic_metadata.h
> > @@ -23,6 +23,8 @@
> >
> >  #include "frame.h"
> >  #include "rational.h"
> > +#include "libavcodec/get_bits.h"
> > +#include "libavcodec/put_bits.h"
>
> As the header doesn't use functions from these, but the implementation
> does, they should be in libavutil/hdr_dynamic_metadata.c instead.
>
Right. Changed.

>
> >      /**
> >       * The nominal maximum display luminance of the targeted system
> display,
> > -     * in units of 0.0001 candelas per square metre. The value shall be
> in
> > +     * in units of 1 candelas per square metre. The value shall be in
> >       * the range of 0 to 10000, inclusive.
> >       */
>
> This fix is probably not strictly related to your change?
>
Right. I have changed ffmpeg locally to pass through HDR10+ in ffmpeg. I
have to split my changes to smaller CLs for review. So, some changes like
this may not strictly related, but required for my next changes. Here, the
comment explanation needs correction anyway.

>
> Cheers,
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".