| Message ID | 20200210194408.251496-1-moh.izadi@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Update HDR10+ metadata structure. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Hello, What's the status of the cl review? Is it good to submit? -- Best, Mohammad On Mon, Feb 10, 2020 at 11:44 AM Mohammad Izadi <moh.izadi@gmail.com> wrote: > From: Mohammad Izadi <izadi@google.com> > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > data in the follow-up CLs. > --- > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > libavutil/hdr_dynamic_metadata.h | 13 ++- > libavutil/version.h | 2 +- > 3 files changed, 178 insertions(+), 2 deletions(-) > > diff --git a/libavutil/hdr_dynamic_metadata.c > b/libavutil/hdr_dynamic_metadata.c > index 0fa1ee82de..f24bcb40f5 100644 > --- a/libavutil/hdr_dynamic_metadata.c > +++ b/libavutil/hdr_dynamic_metadata.c > @@ -19,8 +19,17 @@ > */ > > #include "hdr_dynamic_metadata.h" > +#include "libavcodec/get_bits.h" > #include "mem.h" > > +static const int32_t luminance_den = 1; > +static const int32_t peak_luminance_den = 15; > +static const int32_t rgb_den = 100000; > +static const int32_t fraction_pixel_den = 1000; > +static const int32_t knee_point_den = 4095; > +static const int32_t bezier_anchor_den = 1023; > +static const int32_t saturation_weight_den = 8; > + > AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) > { > AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); > @@ -45,3 +54,159 @@ AVDynamicHDRPlus > *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) > > return (AVDynamicHDRPlus *)side_data->data; > } > + > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > + AVDynamicHDRPlus *s) > +{ > + int w, i, j; > + GetBitContext gb; > + if (!data) > + return AVERROR_INVALIDDATA; > + > + int ret = init_get_bits8(&gb, data, size); > + if (ret < 0) > + return AVERROR_INVALIDDATA; > + > + 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; > + > + if (get_bits_left(&gb) < ((19 * 8 + 1) * (s->num_windows - 1))) > + return AVERROR_INVALIDDATA; > + for (w = 1; w < s->num_windows; w++) { > + s->params[w].window_upper_left_corner_x.num = get_bits(&gb, 16); > + s->params[w].window_upper_left_corner_y.num = get_bits(&gb, 16); > + s->params[w].window_lower_right_corner_x.num = get_bits(&gb, 16); > + s->params[w].window_lower_right_corner_y.num = get_bits(&gb, 16); > + // The corners are set to absolute coordinates here. They should > be > + // converted to the relative coordinates (in [0, 1]) in the > decoder. > + s->params[w].window_upper_left_corner_x.den = 1; > + s->params[w].window_upper_left_corner_y.den = 1; > + s->params[w].window_lower_right_corner_x.den = 1; > + s->params[w].window_lower_right_corner_y.den = 1; > + > + s->params[w].center_of_ellipse_x = get_bits(&gb, 16); > + s->params[w].center_of_ellipse_y = get_bits(&gb, 16); > + s->params[w].rotation_angle = get_bits(&gb, 8); > + s->params[w].semimajor_axis_internal_ellipse = get_bits(&gb, 16); > + s->params[w].semimajor_axis_external_ellipse = get_bits(&gb, 16); > + s->params[w].semiminor_axis_external_ellipse = get_bits(&gb, 16); > + s->params[w].overlap_process_option = get_bits1(&gb); > + } > + > + if (get_bits_left(&gb) < 28) > + return AVERROR_INVALIDDATA; > + s->targeted_system_display_maximum_luminance.num = get_bits(&gb, 27); > + s->targeted_system_display_maximum_luminance.den = luminance_den; > + s->targeted_system_display_actual_peak_luminance_flag = > get_bits1(&gb); > + > + if (s->targeted_system_display_actual_peak_luminance_flag) { > + int rows, cols; > + if (get_bits_left(&gb) < 10) > + return AVERROR_INVALIDDATA; > + rows = get_bits(&gb, 5); > + cols = get_bits(&gb, 5); > + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) > + return AVERROR_INVALIDDATA; > + > + s->num_rows_targeted_system_display_actual_peak_luminance = rows; > + s->num_cols_targeted_system_display_actual_peak_luminance = cols; > + > + if (get_bits_left(&gb) < (rows * cols * 4)) > + return AVERROR_INVALIDDATA; > + > + for (i = 0; i < rows; i++) { > + for (j = 0; j < cols; j++) { > + > s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(&gb, > 4); > + > s->targeted_system_display_actual_peak_luminance[i][j].den = > peak_luminance_den; > + } > + } > + } > + for (w = 0; w < s->num_windows; w++) { > + if (get_bits_left(&gb) < (3 * 17 + 17 + 4)) > + return AVERROR_INVALIDDATA; > + for (i = 0; i < 3; i++) { > + s->params[w].maxscl[i].num = get_bits(&gb, 17); > + s->params[w].maxscl[i].den = rgb_den; > + } > + s->params[w].average_maxrgb.num = get_bits(&gb, 17); > + s->params[w].average_maxrgb.den = rgb_den; > + s->params[w].num_distribution_maxrgb_percentiles = get_bits(&gb, > 4); > + > + if (get_bits_left(&gb) < > + (s->params[w].num_distribution_maxrgb_percentiles * 24)) > + return AVERROR_INVALIDDATA; > + for (i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; > i++) { > + s->params[w].distribution_maxrgb[i].percentage = > get_bits(&gb, 7); > + s->params[w].distribution_maxrgb[i].percentile.num = > get_bits(&gb, 17); > + s->params[w].distribution_maxrgb[i].percentile.den = rgb_den; > + } > + > + if (get_bits_left(&gb) < 10) > + return AVERROR_INVALIDDATA; > + s->params[w].fraction_bright_pixels.num = get_bits(&gb, 10); > + s->params[w].fraction_bright_pixels.den = fraction_pixel_den; > + } > + if (get_bits_left(&gb) < 1) > + return AVERROR_INVALIDDATA; > + s->mastering_display_actual_peak_luminance_flag = get_bits1(&gb); > + if (s->mastering_display_actual_peak_luminance_flag) { > + int rows, cols; > + if (get_bits_left(&gb) < 10) > + return AVERROR_INVALIDDATA; > + rows = get_bits(&gb, 5); > + cols = get_bits(&gb, 5); > + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) > + return AVERROR_INVALIDDATA; > + > + s->num_rows_mastering_display_actual_peak_luminance = rows; > + s->num_cols_mastering_display_actual_peak_luminance = cols; > + > + if (get_bits_left(&gb) < (rows * cols * 4)) > + return AVERROR_INVALIDDATA; > + > + for (i = 0; i < rows; i++) { > + for (j = 0; j < cols; j++) { > + s->mastering_display_actual_peak_luminance[i][j].num = > get_bits(&gb, 4); > + s->mastering_display_actual_peak_luminance[i][j].den = > peak_luminance_den; > + } > + } > + } > + > + for (w = 0; w < s->num_windows; w++) { > + if (get_bits_left(&gb) < 1) > + return AVERROR_INVALIDDATA; > + s->params[w].tone_mapping_flag = get_bits1(&gb); > + if (s->params[w].tone_mapping_flag) { > + if (get_bits_left(&gb) < 28) > + return AVERROR_INVALIDDATA; > + s->params[w].knee_point_x.num = get_bits(&gb, 12); > + s->params[w].knee_point_x.den = knee_point_den; > + s->params[w].knee_point_y.num = get_bits(&gb, 12); > + s->params[w].knee_point_y.den = knee_point_den; > + s->params[w].num_bezier_curve_anchors = get_bits(&gb, 4); > + > + if (get_bits_left(&gb) < > (s->params[w].num_bezier_curve_anchors * 10)) > + return AVERROR_INVALIDDATA; > + for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) { > + s->params[w].bezier_curve_anchors[i].num = get_bits(&gb, > 10); > + s->params[w].bezier_curve_anchors[i].den = > bezier_anchor_den; > + } > + } > + > + if (get_bits_left(&gb) < 1) > + return AVERROR_INVALIDDATA; > + s->params[w].color_saturation_mapping_flag = get_bits1(&gb); > + if (s->params[w].color_saturation_mapping_flag) { > + if (get_bits_left(&gb) < 6) > + return AVERROR_INVALIDDATA; > + s->params[w].color_saturation_weight.num = get_bits(&gb, 6); > + s->params[w].color_saturation_weight.den = > saturation_weight_den; > + } > + } > + > + return 0; > +} > diff --git a/libavutil/hdr_dynamic_metadata.h > b/libavutil/hdr_dynamic_metadata.h > index 2d72de56ae..107df4bacf 100644 > --- a/libavutil/hdr_dynamic_metadata.h > +++ b/libavutil/hdr_dynamic_metadata.h > @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { > > /** > * 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. > */ > AVRational targeted_system_display_maximum_luminance; > @@ -340,4 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t > *size); > */ > AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); > > +/** > + * Decode the payload of SMPTE ST 2094-40 (coming from the user data > registered > + * ITU-T T.35) to AVDynamicHDRPlus. > + * @param data The payload of SMPTE ST 2094-40 extracted from the user > data registered ITU-T T.35. > + * @param size The size of the payload of SMPTE ST 2094-40. > + * @param s The decoded AVDynamicHDRPlus structure. > + * > + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. > + */ > +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, > AVDynamicHDRPlus *s); > + > #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ > diff --git a/libavutil/version.h b/libavutil/version.h > index af8f614aff..2bc1b98615 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -79,7 +79,7 @@ > */ > > #define LIBAVUTIL_VERSION_MAJOR 56 > -#define LIBAVUTIL_VERSION_MINOR 38 > +#define LIBAVUTIL_VERSION_MINOR 39 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > -- > 2.25.0.341.g760bfbb309-goog > >
On 10/02/2020 19:44, Mohammad Izadi wrote: > /** > * 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. > */ > AVRational targeted_system_display_maximum_luminance; Is this comment change related to a behavior change (thus an API break), or was it always 1, and the comment was wrong? - Derek
The comment was wrong. -- Best, Mohammad On Wed, Feb 19, 2020 at 7:22 AM Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 10/02/2020 19:44, Mohammad Izadi wrote: > > /** > > * 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. > > */ > > AVRational targeted_system_display_maximum_luminance; > > Is this comment change related to a behavior change (thus an API > break), or was it always 1, and the comment was wrong? > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 19/02/2020 18:25, Mohammad Izadi wrote:
> The comment was wrong.
Thanks for clarifying!
- Derek
On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > From: Mohammad Izadi <izadi@google.com> > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > data in the follow-up CLs. > --- > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > libavutil/hdr_dynamic_metadata.h | 13 ++- > libavutil/version.h | 2 +- > 3 files changed, 178 insertions(+), 2 deletions(-) > > diff --git a/libavutil/hdr_dynamic_metadata.c > b/libavutil/hdr_dynamic_metadata.c > index 0fa1ee82de..f24bcb40f5 100644 > --- a/libavutil/hdr_dynamic_metadata.c > +++ b/libavutil/hdr_dynamic_metadata.c > @@ -19,8 +19,17 @@ > */ > > #include "hdr_dynamic_metadata.h" > +#include "libavcodec/get_bits.h" > wait is it ok to use libavcodec headers in libavutil? while it's fine at compilation time since it is an inlined header, I don't think it's a good idea to freely use such functions in this way: what I would do is rather implement the parsing in libavcodec, store the fields in a structure defined in libavutil and then use this new structure in here > #include "mem.h" > > +static const int32_t luminance_den = 1; > +static const int32_t peak_luminance_den = 15; > +static const int32_t rgb_den = 100000; > +static const int32_t fraction_pixel_den = 1000; > +static const int32_t knee_point_den = 4095; > +static const int32_t bezier_anchor_den = 1023; > +static const int32_t saturation_weight_den = 8; > These fields could also be stored in the structure i mentioned, rather then having them declared as static here.
On Thu, Feb 20, 2020 at 12:10 PM Vittorio Giovara < vittorio.giovara@gmail.com> wrote: > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > From: Mohammad Izadi <izadi@google.com> > > > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > > data in the follow-up CLs. > > --- > > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > > libavutil/hdr_dynamic_metadata.h | 13 ++- > > libavutil/version.h | 2 +- > > 3 files changed, 178 insertions(+), 2 deletions(-) > > > > diff --git a/libavutil/hdr_dynamic_metadata.c > > b/libavutil/hdr_dynamic_metadata.c > > index 0fa1ee82de..f24bcb40f5 100644 > > --- a/libavutil/hdr_dynamic_metadata.c > > +++ b/libavutil/hdr_dynamic_metadata.c > > @@ -19,8 +19,17 @@ > > */ > > > > #include "hdr_dynamic_metadata.h" > > +#include "libavcodec/get_bits.h" > > > > wait is it ok to use libavcodec headers in libavutil? while it's fine at > compilation time since it is an inlined header, I don't think it's a good > idea to freely use such functions in this way: what I would do is rather > implement the parsing in libavcodec, store the fields in a structure > defined in libavutil and then use this new structure in here > > What if I move it all to livavcodec? > > > #include "mem.h" > > > > +static const int32_t luminance_den = 1; > > +static const int32_t peak_luminance_den = 15; > > +static const int32_t rgb_den = 100000; > > +static const int32_t fraction_pixel_den = 1000; > > +static const int32_t knee_point_den = 4095; > > +static const int32_t bezier_anchor_den = 1023; > > +static const int32_t saturation_weight_den = 8; > > > > These fields could also be stored in the structure i mentioned, rather then > having them declared as static here. > You mean to create a new struct just for constants? > -- > Vittorio > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, Feb 20, 2020 at 3:16 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > On Thu, Feb 20, 2020 at 12:10 PM Vittorio Giovara < > vittorio.giovara@gmail.com> wrote: > > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> > > wrote: > > > > > From: Mohammad Izadi <izadi@google.com> > > > > > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > > > data in the follow-up CLs. > > > --- > > > libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > > > libavutil/hdr_dynamic_metadata.h | 13 ++- > > > libavutil/version.h | 2 +- > > > 3 files changed, 178 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavutil/hdr_dynamic_metadata.c > > > b/libavutil/hdr_dynamic_metadata.c > > > index 0fa1ee82de..f24bcb40f5 100644 > > > --- a/libavutil/hdr_dynamic_metadata.c > > > +++ b/libavutil/hdr_dynamic_metadata.c > > > @@ -19,8 +19,17 @@ > > > */ > > > > > > #include "hdr_dynamic_metadata.h" > > > +#include "libavcodec/get_bits.h" > > > > > > > wait is it ok to use libavcodec headers in libavutil? while it's fine at > > compilation time since it is an inlined header, I don't think it's a good > > idea to freely use such functions in this way: what I would do is rather > > implement the parsing in libavcodec, store the fields in a structure > > defined in libavutil and then use this new structure in here > > > > What if I move it all to livavcodec? > > that works too, but then you'd need to figure out a way to export this information to the callers I suppose See how it is done for the various SEI handling functions, that parse fields in h264*.c or hevc*.c, call a lavu function, and then export such information with a frame side data. (note that you're replying with some formatting on, mixing your reply with the previous one) > > > > > #include "mem.h" > > > > > > +static const int32_t luminance_den = 1; > > > +static const int32_t peak_luminance_den = 15; > > > +static const int32_t rgb_den = 100000; > > > +static const int32_t fraction_pixel_den = 1000; > > > +static const int32_t knee_point_den = 4095; > > > +static const int32_t bezier_anchor_den = 1023; > > > +static const int32_t saturation_weight_den = 8; > > > > > > > These fields could also be stored in the structure i mentioned, rather > then > > having them declared as static here. > > > You mean to create a new struct just for constants? > no i guess i don't understand why those constants are needed there: if they are just used as constant you could just use #define instead of static int, if they are used once, you could use them where needed, and avoid declaring them in the first place
On 2/20/2020 5:02 PM, Vittorio Giovara wrote: > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > >> From: Mohammad Izadi <izadi@google.com> >> >> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side >> data in the follow-up CLs. >> --- >> libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ >> libavutil/hdr_dynamic_metadata.h | 13 ++- >> libavutil/version.h | 2 +- >> 3 files changed, 178 insertions(+), 2 deletions(-) >> >> diff --git a/libavutil/hdr_dynamic_metadata.c >> b/libavutil/hdr_dynamic_metadata.c >> index 0fa1ee82de..f24bcb40f5 100644 >> --- a/libavutil/hdr_dynamic_metadata.c >> +++ b/libavutil/hdr_dynamic_metadata.c >> @@ -19,8 +19,17 @@ >> */ >> >> #include "hdr_dynamic_metadata.h" >> +#include "libavcodec/get_bits.h" >> > > wait is it ok to use libavcodec headers in libavutil? while it's fine at > compilation time since it is an inlined header, I don't think it's a good > idea to freely use such functions in this way: what I would do is rather > implement the parsing in libavcodec, store the fields in a structure > defined in libavutil and then use this new structure in here This seems excessive only to use a bitstream reader to read a bunch of fields in a buffer. get_bits.h is included in lavf, so i don't see why it couldn't be used in lavu. As you said it's inlined. > > >> #include "mem.h" >> >> +static const int32_t luminance_den = 1; >> +static const int32_t peak_luminance_den = 15; >> +static const int32_t rgb_den = 100000; >> +static const int32_t fraction_pixel_den = 1000; >> +static const int32_t knee_point_den = 4095; >> +static const int32_t bezier_anchor_den = 1023; >> +static const int32_t saturation_weight_den = 8; >> > > These fields could also be stored in the structure i mentioned, rather then > having them declared as static here. >
On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote: > On 2/20/2020 5:02 PM, Vittorio Giovara wrote: > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > >> From: Mohammad Izadi <izadi@google.com> > >> > >> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > >> data in the follow-up CLs. > >> --- > >> libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > >> libavutil/hdr_dynamic_metadata.h | 13 ++- > >> libavutil/version.h | 2 +- > >> 3 files changed, 178 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavutil/hdr_dynamic_metadata.c > >> b/libavutil/hdr_dynamic_metadata.c > >> index 0fa1ee82de..f24bcb40f5 100644 > >> --- a/libavutil/hdr_dynamic_metadata.c > >> +++ b/libavutil/hdr_dynamic_metadata.c > >> @@ -19,8 +19,17 @@ > >> */ > >> > >> #include "hdr_dynamic_metadata.h" > >> +#include "libavcodec/get_bits.h" > >> > > > > wait is it ok to use libavcodec headers in libavutil? while it's fine at > > compilation time since it is an inlined header, I don't think it's a good > > idea to freely use such functions in this way: what I would do is rather > > implement the parsing in libavcodec, store the fields in a structure > > defined in libavutil and then use this new structure in here > > This seems excessive only to use a bitstream reader to read a bunch of > fields in a buffer. > > get_bits.h is included in lavf, so i don't see why it couldn't be used > in lavu. bacuase lavf is a library which depends on lavc, not vice versa, hierarchy is respected As you said it's inlined. > I still consider it a poor design choice: either make bitstream reader public in a separate library (not easy, i am aware) or do the bitstream reading where the bistream actually is, IMO Vittorio > > > > > >> #include "mem.h" > >> > >> +static const int32_t luminance_den = 1; > >> +static const int32_t peak_luminance_den = 15; > >> +static const int32_t rgb_den = 100000; > >> +static const int32_t fraction_pixel_den = 1000; > >> +static const int32_t knee_point_den = 4095; > >> +static const int32_t bezier_anchor_den = 1023; > >> +static const int32_t saturation_weight_den = 8; > >> > > > > These fields could also be stored in the structure i mentioned, rather > then > > having them declared as static here. > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Vittorio Giovara (12020-02-20): > bacuase lavf is a library which depends on lavc, not vice versa, hierarchy > is respected > > I still consider it a poor design choice The poor design choice is to keep the libraries separated. We can see all the trouble it causes us, between this discussion and all the avpriv_ symbols. I have yet to see any actual benefit. Regards,
On Fri, Feb 21, 2020 at 5:59 AM Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > > On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote: > > > On 2/20/2020 5:02 PM, Vittorio Giovara wrote: > > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> > > wrote: > > > > > >> From: Mohammad Izadi <izadi@google.com> > > >> > > >> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side > > >> data in the follow-up CLs. > > >> --- > > >> libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ > > >> libavutil/hdr_dynamic_metadata.h | 13 ++- > > >> libavutil/version.h | 2 +- > > >> 3 files changed, 178 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/libavutil/hdr_dynamic_metadata.c > > >> b/libavutil/hdr_dynamic_metadata.c > > >> index 0fa1ee82de..f24bcb40f5 100644 > > >> --- a/libavutil/hdr_dynamic_metadata.c > > >> +++ b/libavutil/hdr_dynamic_metadata.c > > >> @@ -19,8 +19,17 @@ > > >> */ > > >> > > >> #include "hdr_dynamic_metadata.h" > > >> +#include "libavcodec/get_bits.h" > > >> > > > > > > wait is it ok to use libavcodec headers in libavutil? while it's fine at > > > compilation time since it is an inlined header, I don't think it's a good > > > idea to freely use such functions in this way: what I would do is rather > > > implement the parsing in libavcodec, store the fields in a structure > > > defined in libavutil and then use this new structure in here > > > > This seems excessive only to use a bitstream reader to read a bunch of > > fields in a buffer. > > > > get_bits.h is included in lavf, so i don't see why it couldn't be used > > in lavu. > > > bacuase lavf is a library which depends on lavc, not vice versa, hierarchy > is respected > > As you said it's inlined. > > > > I still consider it a poor design choice: either make bitstream reader > public in a separate library (not easy, i am aware) or do the bitstream > reading where the bistream actually is, IMO > I agree that the parsing should just move to avcodec. We parse every other SEI straight in the codec it comes from, why does this one have parsing in avutil? It seems weird. - Hendrik
If you believe lavc is at the top of the hierarchy, I can simply move the file to lavc. Then both lavc and lavf can use it and hierarchy is respected. Can I do that? Doesn't break API? Any objection (with solution)? Let's make right decisions to speed up the process please :). -- Best, Mohammad On Fri, Feb 21, 2020 at 2:53 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Fri, Feb 21, 2020 at 5:59 AM Vittorio Giovara > <vittorio.giovara@gmail.com> wrote: > > > > On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote: > > > > > On 2/20/2020 5:02 PM, Vittorio Giovara wrote: > > > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> > > > wrote: > > > > > > > >> From: Mohammad Izadi <izadi@google.com> > > > >> > > > >> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet > side > > > >> data in the follow-up CLs. > > > >> --- > > > >> libavutil/hdr_dynamic_metadata.c | 165 > +++++++++++++++++++++++++++++++ > > > >> libavutil/hdr_dynamic_metadata.h | 13 ++- > > > >> libavutil/version.h | 2 +- > > > >> 3 files changed, 178 insertions(+), 2 deletions(-) > > > >> > > > >> diff --git a/libavutil/hdr_dynamic_metadata.c > > > >> b/libavutil/hdr_dynamic_metadata.c > > > >> index 0fa1ee82de..f24bcb40f5 100644 > > > >> --- a/libavutil/hdr_dynamic_metadata.c > > > >> +++ b/libavutil/hdr_dynamic_metadata.c > > > >> @@ -19,8 +19,17 @@ > > > >> */ > > > >> > > > >> #include "hdr_dynamic_metadata.h" > > > >> +#include "libavcodec/get_bits.h" > > > >> > > > > > > > > wait is it ok to use libavcodec headers in libavutil? while it's > fine at > > > > compilation time since it is an inlined header, I don't think it's a > good > > > > idea to freely use such functions in this way: what I would do is > rather > > > > implement the parsing in libavcodec, store the fields in a structure > > > > defined in libavutil and then use this new structure in here > > > > > > This seems excessive only to use a bitstream reader to read a bunch of > > > fields in a buffer. > > > > > > get_bits.h is included in lavf, so i don't see why it couldn't be used > > > in lavu. > > > > > > bacuase lavf is a library which depends on lavc, not vice versa, > hierarchy > > is respected > > > > As you said it's inlined. > > > > > > > I still consider it a poor design choice: either make bitstream reader > > public in a separate library (not easy, i am aware) or do the bitstream > > reading where the bistream actually is, IMO > > > > I agree that the parsing should just move to avcodec. We parse every > other SEI straight in the codec it comes from, why does this one have > parsing in avutil? It seems weird. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > > If you believe lavc is at the top of the hierarchy, I can simply move the > file to lavc. Then both lavc and lavf can use it and hierarchy is > respected. Can I do that? Doesn't break API? Any objection (with solution)? > Let's make right decisions to speed up the process please :). > -- The struct itself belongs in lavu with everything else of AVFrame. The parsing of the mpeg-specific SEI data belongs in avcodec. You can't just blindly move everything. - Hendrik
Why does the struct belong to lavu? This struct is super similar to structs in libavcodec/hevc_sei.h. We just move it to a new file to share it between hevc and vp9 encoder/decoder. -- Best, Mohammad On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > > If you believe lavc is at the top of the hierarchy, I can simply move the > > file to lavc. Then both lavc and lavf can use it and hierarchy is > > respected. Can I do that? Doesn't break API? Any objection (with > solution)? > > Let's make right decisions to speed up the process please :). > > -- > > > The struct itself belongs in lavu with everything else of AVFrame. The > parsing of the mpeg-specific SEI data belongs in avcodec. You can't > just blindly move everything. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > Why does the struct belong to lavu? This struct is super similar to structs > in libavcodec/hevc_sei.h. We just move it to a new file to share it between > hevc and vp9 encoder/decoder. > > -- > 1. Please kindly stop top posting: http://www.idallen.com/topposting.html 2. It belongs to lavu because it's where the frame code generically code is. I'm not familiar with this API too much, but from what i gather users may need to have a way of accessing this data without pulling in all the dependencies of lavc or lavf. Vittorio On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com> > > wrote: > > > > > > If you believe lavc is at the top of the hierarchy, I can simply move > the > > > file to lavc. Then both lavc and lavf can use it and hierarchy is > > > respected. Can I do that? Doesn't break API? Any objection (with > > solution)? > > > Let's make right decisions to speed up the process please :). > > > -- > > > > > > The struct itself belongs in lavu with everything else of AVFrame. The > > parsing of the mpeg-specific SEI data belongs in avcodec. You can't > > just blindly move everything. > > > > - Hendrik > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > Why does the struct belong to lavu? This struct is super similar to > structs > > in libavcodec/hevc_sei.h. We just move it to a new file to share it > between > > hevc and vp9 encoder/decoder. > > > > -- > > > > 1. Please kindly stop top posting: http://www.idallen.com/topposting.html > 2. It belongs to lavu because it's where the frame code generically code > is. I'm not familiar with this API too much, but from what i gather users > may need to have a way of accessing this data without pulling in all the > dependencies of lavc or lavf. > This struct is related to parsing and SEI, not frame. If so, why other structs are not in lavu? Please check similar structs in hevc_sei? > > Vittorio > > On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com> > wrote: > > > > > On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com> > > > wrote: > > > > > > > > If you believe lavc is at the top of the hierarchy, I can simply move > > the > > > > file to lavc. Then both lavc and lavf can use it and hierarchy is > > > > respected. Can I do that? Doesn't break API? Any objection (with > > > solution)? > > > > Let's make right decisions to speed up the process please :). > > > > -- > > > > > > > > > The struct itself belongs in lavu with everything else of AVFrame. The > > > parsing of the mpeg-specific SEI data belongs in avcodec. You can't > > > just blindly move everything. > > > > > > - Hendrik > > > _______________________________________________ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > To unsubscribe, visit link above, or email > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > > -- > Vittorio > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, Feb 22, 2020 at 12:44 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara <vittorio.giovara@gmail.com > > > wrote: > > > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> > > wrote: > > > > > Why does the struct belong to lavu? This struct is super similar to > > structs > > > in libavcodec/hevc_sei.h. We just move it to a new file to share it > > between > > > hevc and vp9 encoder/decoder. > > > > > > -- > > > > > > > 1. Please kindly stop top posting: > http://www.idallen.com/topposting.html > > 2. It belongs to lavu because it's where the frame code generically code > > is. I'm not familiar with this API too much, but from what i gather users > > may need to have a way of accessing this data without pulling in all the > > dependencies of lavc or lavf. > > > This struct is related to parsing and SEI, not frame. If so, why other > structs are not in lavu? Please check similar structs in hevc_sei? > I don't think I understand your question, but if you need examples you can check these patches 8f58ecc344a92e63193c38e28c173be987954bbb structure defined in lavu, e7a6f8c972a0b5b98ef7bbf393e95c434e9e2539 structure populated in lavc d91718107c33960ad295950d7419e6dba292d723 structure defined in lavu, used in lavc 7e244c68600f479270e979258e389ed5240885fb same and so on and so on, so I'd advise you do to the same, scrapping your current code if necessary.
On Mon, Feb 24, 2020 at 9:56 AM Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > On Sat, Feb 22, 2020 at 12:44 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara < > vittorio.giovara@gmail.com > > > > > wrote: > > > > > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> > > > wrote: > > > > > > > Why does the struct belong to lavu? This struct is super similar to > > > structs > > > > in libavcodec/hevc_sei.h. We just move it to a new file to share it > > > between > > > > hevc and vp9 encoder/decoder. > > > > > > > > -- > > > > > > > > > > 1. Please kindly stop top posting: > > http://www.idallen.com/topposting.html > > > 2. It belongs to lavu because it's where the frame code generically > code > > > is. I'm not familiar with this API too much, but from what i gather > users > > > may need to have a way of accessing this data without pulling in all > the > > > dependencies of lavc or lavf. > > > > > This struct is related to parsing and SEI, not frame. If so, why other > > structs are not in lavu? Please check similar structs in hevc_sei? > > > > I don't think I understand your question, but if you need examples you can > check these patches > 8f58ecc344a92e63193c38e28c173be987954bbb structure defined in lavu, > e7a6f8c972a0b5b98ef7bbf393e95c434e9e2539 structure populated in lavc > d91718107c33960ad295950d7419e6dba292d723 structure defined in lavu, used in > lavc > 7e244c68600f479270e979258e389ed5240885fb same > and so on and so on, so I'd advise you do to the same, scrapping your > current code if necessary. > I will do, but let me explain the problem in more details and you may help me for a solution. The patches you mentioned, contains two structs AVSphericalMapping and AVMasteringDisplayMetadata in lavu. They are easily set (afew members) in lavc. The struct for HDR10+ is very similar and I would keep it in lavu. But, we have to parse and decode a message and then populate the values. Your structs are simple and no need for parsing them in lavc. So, my struct needs two steps : 1) parsing/encoding/decoding and 2) populating. It is not a good idea to implement the 2 steps for each codec separately. Instead it would be better to implement once and reuse them as both steps are long and complex. Now please advise me where is better to put 1 and 2 in lavc. Right now, I have all with struct in lavu. > -- > Vittorio > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, Feb 25, 2020 at 9:16 PM Mohammad Izadi <moh.izadi@gmail.com> wrote: > On Mon, Feb 24, 2020 at 9:56 AM Vittorio Giovara < > vittorio.giovara@gmail.com> > wrote: > > > On Sat, Feb 22, 2020 at 12:44 PM Mohammad Izadi <moh.izadi@gmail.com> > > wrote: > > > > > On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara < > > vittorio.giovara@gmail.com > > > > > > > wrote: > > > > > > > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> > > > > wrote: > > > > > > > > > Why does the struct belong to lavu? This struct is super similar to > > > > structs > > > > > in libavcodec/hevc_sei.h. We just move it to a new file to share it > > > > between > > > > > hevc and vp9 encoder/decoder. > > > > > > > > > > -- > > > > > > > > > > > > > 1. Please kindly stop top posting: > > > http://www.idallen.com/topposting.html > > > > 2. It belongs to lavu because it's where the frame code generically > > code > > > > is. I'm not familiar with this API too much, but from what i gather > > users > > > > may need to have a way of accessing this data without pulling in all > > the > > > > dependencies of lavc or lavf. > > > > > > > This struct is related to parsing and SEI, not frame. If so, why other > > > structs are not in lavu? Please check similar structs in hevc_sei? > > > > > > > I don't think I understand your question, but if you need examples you > can > > check these patches > > 8f58ecc344a92e63193c38e28c173be987954bbb structure defined in lavu, > > e7a6f8c972a0b5b98ef7bbf393e95c434e9e2539 structure populated in lavc > > d91718107c33960ad295950d7419e6dba292d723 structure defined in lavu, used > in > > lavc > > 7e244c68600f479270e979258e389ed5240885fb same > > and so on and so on, so I'd advise you do to the same, scrapping your > > current code if necessary. > > > I will do, but let me explain the problem in more details and you may help > me for a solution. The patches you mentioned, contains two structs > AVSphericalMapping > and AVMasteringDisplayMetadata in lavu. They are easily set (afew members) > in lavc. The struct for HDR10+ is very similar and I would keep it in lavu. > But, we have to parse and decode a message and then populate the values. > Your structs are simple and no need for parsing them in lavc. > So, my struct needs two steps : 1) parsing/encoding/decoding and 2) > populating. It is not a good idea to implement the 2 steps for each codec > separately. Instead it would be better to implement once and reuse them as > both steps are long and complex. Now please advise me where is better to > put 1 and 2 in lavc. Right now, I have all with struct in lavu. > Hi Mohammad, thanks for explaining the problem a bit better. If that's the case you could have an helper function that parses the data in lavc (usually these functions are prefixed with ff_, meaning their intended use is internal within a library) and use the helper function to parse whatever buffer you pass. This wrapper could then return a lavu struct to be embedded in a side data message like in the examples I sent you. Let me know if this is clear enough for you Thanks
On Tue, Feb 25, 2020, 9:32 PM Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > On Tue, Feb 25, 2020 at 9:16 PM Mohammad Izadi <moh.izadi@gmail.com> > wrote: > > > On Mon, Feb 24, 2020 at 9:56 AM Vittorio Giovara < > > vittorio.giovara@gmail.com> > > wrote: > > > > > On Sat, Feb 22, 2020 at 12:44 PM Mohammad Izadi <moh.izadi@gmail.com> > > > wrote: > > > > > > > On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara < > > > vittorio.giovara@gmail.com > > > > > > > > > wrote: > > > > > > > > > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi < > moh.izadi@gmail.com> > > > > > wrote: > > > > > > > > > > > Why does the struct belong to lavu? This struct is super similar > to > > > > > structs > > > > > > in libavcodec/hevc_sei.h. We just move it to a new file to share > it > > > > > between > > > > > > hevc and vp9 encoder/decoder. > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > 1. Please kindly stop top posting: > > > > http://www.idallen.com/topposting.html > > > > > 2. It belongs to lavu because it's where the frame code generically > > > code > > > > > is. I'm not familiar with this API too much, but from what i gather > > > users > > > > > may need to have a way of accessing this data without pulling in > all > > > the > > > > > dependencies of lavc or lavf. > > > > > > > > > This struct is related to parsing and SEI, not frame. If so, why > other > > > > structs are not in lavu? Please check similar structs in hevc_sei? > > > > > > > > > > I don't think I understand your question, but if you need examples you > > can > > > check these patches > > > 8f58ecc344a92e63193c38e28c173be987954bbb structure defined in lavu, > > > e7a6f8c972a0b5b98ef7bbf393e95c434e9e2539 structure populated in lavc > > > d91718107c33960ad295950d7419e6dba292d723 structure defined in lavu, > used > > in > > > lavc > > > 7e244c68600f479270e979258e389ed5240885fb same > > > and so on and so on, so I'd advise you do to the same, scrapping your > > > current code if necessary. > > > > > I will do, but let me explain the problem in more details and you may > help > > me for a solution. The patches you mentioned, contains two structs > > AVSphericalMapping > > and AVMasteringDisplayMetadata in lavu. They are easily set (afew > members) > > in lavc. The struct for HDR10+ is very similar and I would keep it in > lavu. > > But, we have to parse and decode a message and then populate the values. > > Your structs are simple and no need for parsing them in lavc. > > So, my struct needs two steps : 1) parsing/encoding/decoding and 2) > > populating. It is not a good idea to implement the 2 steps for each codec > > separately. Instead it would be better to implement once and reuse them > as > > both steps are long and complex. Now please advise me where is better to > > put 1 and 2 in lavc. Right now, I have all with struct in lavu. > > > > Hi Mohammad, > thanks for explaining the problem a bit better. If that's the case you > could have an helper function that parses the data in lavc (usually these > functions are prefixed with ff_, meaning their intended use is internal > within a library) and use the helper function to parse whatever buffer you > pass. This wrapper could then return a lavu struct to be embedded in a side > data message like in the examples I sent you. > Let me know if this is clear enough for you > Thanks > Thanks for your solution. I have to use the parser or helper function in libavformat for mkv too. Am I allowed to use the ff_ helpers in lavf? > -- > Vittorio > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c index 0fa1ee82de..f24bcb40f5 100644 --- a/libavutil/hdr_dynamic_metadata.c +++ b/libavutil/hdr_dynamic_metadata.c @@ -19,8 +19,17 @@ */ #include "hdr_dynamic_metadata.h" +#include "libavcodec/get_bits.h" #include "mem.h" +static const int32_t luminance_den = 1; +static const int32_t peak_luminance_den = 15; +static const int32_t rgb_den = 100000; +static const int32_t fraction_pixel_den = 1000; +static const int32_t knee_point_den = 4095; +static const int32_t bezier_anchor_den = 1023; +static const int32_t saturation_weight_den = 8; + AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size) { AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus)); @@ -45,3 +54,159 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame) return (AVDynamicHDRPlus *)side_data->data; } + +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, + AVDynamicHDRPlus *s) +{ + int w, i, j; + GetBitContext gb; + if (!data) + return AVERROR_INVALIDDATA; + + int ret = init_get_bits8(&gb, data, size); + if (ret < 0) + return AVERROR_INVALIDDATA; + + 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; + + if (get_bits_left(&gb) < ((19 * 8 + 1) * (s->num_windows - 1))) + return AVERROR_INVALIDDATA; + for (w = 1; w < s->num_windows; w++) { + s->params[w].window_upper_left_corner_x.num = get_bits(&gb, 16); + s->params[w].window_upper_left_corner_y.num = get_bits(&gb, 16); + s->params[w].window_lower_right_corner_x.num = get_bits(&gb, 16); + s->params[w].window_lower_right_corner_y.num = get_bits(&gb, 16); + // The corners are set to absolute coordinates here. They should be + // converted to the relative coordinates (in [0, 1]) in the decoder. + s->params[w].window_upper_left_corner_x.den = 1; + s->params[w].window_upper_left_corner_y.den = 1; + s->params[w].window_lower_right_corner_x.den = 1; + s->params[w].window_lower_right_corner_y.den = 1; + + s->params[w].center_of_ellipse_x = get_bits(&gb, 16); + s->params[w].center_of_ellipse_y = get_bits(&gb, 16); + s->params[w].rotation_angle = get_bits(&gb, 8); + s->params[w].semimajor_axis_internal_ellipse = get_bits(&gb, 16); + s->params[w].semimajor_axis_external_ellipse = get_bits(&gb, 16); + s->params[w].semiminor_axis_external_ellipse = get_bits(&gb, 16); + s->params[w].overlap_process_option = get_bits1(&gb); + } + + if (get_bits_left(&gb) < 28) + return AVERROR_INVALIDDATA; + s->targeted_system_display_maximum_luminance.num = get_bits(&gb, 27); + s->targeted_system_display_maximum_luminance.den = luminance_den; + s->targeted_system_display_actual_peak_luminance_flag = get_bits1(&gb); + + if (s->targeted_system_display_actual_peak_luminance_flag) { + int rows, cols; + if (get_bits_left(&gb) < 10) + return AVERROR_INVALIDDATA; + rows = get_bits(&gb, 5); + cols = get_bits(&gb, 5); + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) + return AVERROR_INVALIDDATA; + + s->num_rows_targeted_system_display_actual_peak_luminance = rows; + s->num_cols_targeted_system_display_actual_peak_luminance = cols; + + if (get_bits_left(&gb) < (rows * cols * 4)) + return AVERROR_INVALIDDATA; + + for (i = 0; i < rows; i++) { + for (j = 0; j < cols; j++) { + s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(&gb, 4); + s->targeted_system_display_actual_peak_luminance[i][j].den = peak_luminance_den; + } + } + } + for (w = 0; w < s->num_windows; w++) { + if (get_bits_left(&gb) < (3 * 17 + 17 + 4)) + return AVERROR_INVALIDDATA; + for (i = 0; i < 3; i++) { + s->params[w].maxscl[i].num = get_bits(&gb, 17); + s->params[w].maxscl[i].den = rgb_den; + } + s->params[w].average_maxrgb.num = get_bits(&gb, 17); + s->params[w].average_maxrgb.den = rgb_den; + s->params[w].num_distribution_maxrgb_percentiles = get_bits(&gb, 4); + + if (get_bits_left(&gb) < + (s->params[w].num_distribution_maxrgb_percentiles * 24)) + return AVERROR_INVALIDDATA; + for (i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) { + s->params[w].distribution_maxrgb[i].percentage = get_bits(&gb, 7); + s->params[w].distribution_maxrgb[i].percentile.num = get_bits(&gb, 17); + s->params[w].distribution_maxrgb[i].percentile.den = rgb_den; + } + + if (get_bits_left(&gb) < 10) + return AVERROR_INVALIDDATA; + s->params[w].fraction_bright_pixels.num = get_bits(&gb, 10); + s->params[w].fraction_bright_pixels.den = fraction_pixel_den; + } + if (get_bits_left(&gb) < 1) + return AVERROR_INVALIDDATA; + s->mastering_display_actual_peak_luminance_flag = get_bits1(&gb); + if (s->mastering_display_actual_peak_luminance_flag) { + int rows, cols; + if (get_bits_left(&gb) < 10) + return AVERROR_INVALIDDATA; + rows = get_bits(&gb, 5); + cols = get_bits(&gb, 5); + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) + return AVERROR_INVALIDDATA; + + s->num_rows_mastering_display_actual_peak_luminance = rows; + s->num_cols_mastering_display_actual_peak_luminance = cols; + + if (get_bits_left(&gb) < (rows * cols * 4)) + return AVERROR_INVALIDDATA; + + for (i = 0; i < rows; i++) { + for (j = 0; j < cols; j++) { + s->mastering_display_actual_peak_luminance[i][j].num = get_bits(&gb, 4); + s->mastering_display_actual_peak_luminance[i][j].den = peak_luminance_den; + } + } + } + + for (w = 0; w < s->num_windows; w++) { + if (get_bits_left(&gb) < 1) + return AVERROR_INVALIDDATA; + s->params[w].tone_mapping_flag = get_bits1(&gb); + if (s->params[w].tone_mapping_flag) { + if (get_bits_left(&gb) < 28) + return AVERROR_INVALIDDATA; + s->params[w].knee_point_x.num = get_bits(&gb, 12); + s->params[w].knee_point_x.den = knee_point_den; + s->params[w].knee_point_y.num = get_bits(&gb, 12); + s->params[w].knee_point_y.den = knee_point_den; + s->params[w].num_bezier_curve_anchors = get_bits(&gb, 4); + + if (get_bits_left(&gb) < (s->params[w].num_bezier_curve_anchors * 10)) + return AVERROR_INVALIDDATA; + for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) { + s->params[w].bezier_curve_anchors[i].num = get_bits(&gb, 10); + s->params[w].bezier_curve_anchors[i].den = bezier_anchor_den; + } + } + + if (get_bits_left(&gb) < 1) + return AVERROR_INVALIDDATA; + s->params[w].color_saturation_mapping_flag = get_bits1(&gb); + if (s->params[w].color_saturation_mapping_flag) { + if (get_bits_left(&gb) < 6) + return AVERROR_INVALIDDATA; + s->params[w].color_saturation_weight.num = get_bits(&gb, 6); + s->params[w].color_saturation_weight.den = saturation_weight_den; + } + } + + return 0; +} diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h index 2d72de56ae..107df4bacf 100644 --- a/libavutil/hdr_dynamic_metadata.h +++ b/libavutil/hdr_dynamic_metadata.h @@ -265,7 +265,7 @@ typedef struct AVDynamicHDRPlus { /** * 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. */ AVRational targeted_system_display_maximum_luminance; @@ -340,4 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size); */ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); +/** + * Decode the payload of SMPTE ST 2094-40 (coming from the user data registered + * ITU-T T.35) to AVDynamicHDRPlus. + * @param data The payload of SMPTE ST 2094-40 extracted from the user data registered ITU-T T.35. + * @param size The size of the payload of SMPTE ST 2094-40. + * @param s The decoded AVDynamicHDRPlus structure. + * + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR. + */ +int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, AVDynamicHDRPlus *s); + #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ diff --git a/libavutil/version.h b/libavutil/version.h index af8f614aff..2bc1b98615 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 38 +#define LIBAVUTIL_VERSION_MINOR 39 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
From: Mohammad Izadi <izadi@google.com> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side data in the follow-up CLs. --- libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++ libavutil/hdr_dynamic_metadata.h | 13 ++- libavutil/version.h | 2 +- 3 files changed, 178 insertions(+), 2 deletions(-)