[FFmpeg-devel] [PATCH] Update HDR10+ metadata structure.
Moritz Barsnick
barsnick at gmx.net
Wed Feb 5 15:48:26 EET 2020
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.
> + if(!data)
ffmpeg style: "if ("
> + if(ret < 0)
> + return AVERROR_INVALIDDATA;
Ditto.
> + 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.
> --- 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.
> /**
> * 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?
Cheers,
Moritz
More information about the ffmpeg-devel
mailing list