| Message ID | 20180725154640.89762-3-vittorio.giovara@gmail.com |
|---|---|
| State | Accepted |
| Commit | 572ef567a5288d36b8bc2531309710a0e891d35c |
| Headers | show |
On 7/25/2018 12:46 PM, Vittorio Giovara wrote: > --- > libavfilter/colorspace.c | 17 +++++++++++++++++ > libavfilter/colorspace.h | 1 + > libavfilter/vf_tonemap_opencl.c | 19 +------------------ > 3 files changed, 19 insertions(+), 18 deletions(-) > > diff --git a/libavfilter/colorspace.c b/libavfilter/colorspace.c > index d6f6055401..c6682216d6 100644 > --- a/libavfilter/colorspace.c > +++ b/libavfilter/colorspace.c > @@ -118,3 +118,20 @@ double ff_determine_signal_peak(AVFrame *in) > > return peak; > } > + > +void ff_update_hdr_metadata(AVFrame *in, double peak) > +{ > + AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); > + > + if (sd) { > + AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data; > + clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE); > + } > + > + sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); > + if (sd) { > + AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data; > + if (metadata->has_luminance) > + metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000); > + } Frame side data is refcounted, so this is not correct. I'm not sure if a side data element is ever modified in-place long after being created to being with, aside from this function here. Easiest option is to call av_buffer_make_writable() on sd->buf and updating the sd->data pointer before modifying the fields in question, but such manual handling of side data seems improper and fragile. I guess another option is av_mastering_display_metadata_alloc(), copy the values, av_frame_remove_side_data(), av_buffer_create(), then av_frame_new_side_data_from_buf(). Honestly, i think we may have to add a make_writable() function for side data for this.
On 06/08/18 20:21, James Almer wrote: > On 7/25/2018 12:46 PM, Vittorio Giovara wrote: >> --- >> libavfilter/colorspace.c | 17 +++++++++++++++++ >> libavfilter/colorspace.h | 1 + >> libavfilter/vf_tonemap_opencl.c | 19 +------------------ >> 3 files changed, 19 insertions(+), 18 deletions(-) >> >> diff --git a/libavfilter/colorspace.c b/libavfilter/colorspace.c >> index d6f6055401..c6682216d6 100644 >> --- a/libavfilter/colorspace.c >> +++ b/libavfilter/colorspace.c >> @@ -118,3 +118,20 @@ double ff_determine_signal_peak(AVFrame *in) >> >> return peak; >> } >> + >> +void ff_update_hdr_metadata(AVFrame *in, double peak) >> +{ >> + AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); >> + >> + if (sd) { >> + AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data; >> + clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE); >> + } >> + >> + sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); >> + if (sd) { >> + AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data; >> + if (metadata->has_luminance) >> + metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000); >> + } > Frame side data is refcounted, so this is not correct. I'm not sure if a > side data element is ever modified in-place long after being created to > being with, aside from this function here. > > Easiest option is to call av_buffer_make_writable() on sd->buf and > updating the sd->data pointer before modifying the fields in question, > but such manual handling of side data seems improper and fragile. > I guess another option is av_mastering_display_metadata_alloc(), copy > the values, av_frame_remove_side_data(), av_buffer_create(), then > av_frame_new_side_data_from_buf(). > > Honestly, i think we may have to add a make_writable() function for side > data for this. The frame side-data was already copied for the output frame by av_frame_copy_props(), so modifying it here is fine. (I had exactly the same thought when this code appeared in tonemap_opencl.) - Mark
On 8/6/2018 4:52 PM, Mark Thompson wrote: > On 06/08/18 20:21, James Almer wrote: >> On 7/25/2018 12:46 PM, Vittorio Giovara wrote: >>> --- >>> libavfilter/colorspace.c | 17 +++++++++++++++++ >>> libavfilter/colorspace.h | 1 + >>> libavfilter/vf_tonemap_opencl.c | 19 +------------------ >>> 3 files changed, 19 insertions(+), 18 deletions(-) >>> >>> diff --git a/libavfilter/colorspace.c b/libavfilter/colorspace.c >>> index d6f6055401..c6682216d6 100644 >>> --- a/libavfilter/colorspace.c >>> +++ b/libavfilter/colorspace.c >>> @@ -118,3 +118,20 @@ double ff_determine_signal_peak(AVFrame *in) >>> >>> return peak; >>> } >>> + >>> +void ff_update_hdr_metadata(AVFrame *in, double peak) >>> +{ >>> + AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); >>> + >>> + if (sd) { >>> + AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data; >>> + clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE); >>> + } >>> + >>> + sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); >>> + if (sd) { >>> + AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data; >>> + if (metadata->has_luminance) >>> + metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000); >>> + } >> Frame side data is refcounted, so this is not correct. I'm not sure if a >> side data element is ever modified in-place long after being created to >> being with, aside from this function here. >> >> Easiest option is to call av_buffer_make_writable() on sd->buf and >> updating the sd->data pointer before modifying the fields in question, >> but such manual handling of side data seems improper and fragile. >> I guess another option is av_mastering_display_metadata_alloc(), copy >> the values, av_frame_remove_side_data(), av_buffer_create(), then >> av_frame_new_side_data_from_buf(). >> >> Honestly, i think we may have to add a make_writable() function for side >> data for this. > > The frame side-data was already copied for the output frame by av_frame_copy_props(), so modifying it here is fine. (I had exactly the same thought when this code appeared in tonemap_opencl.) > > - Mark Ah, good. I guess the ff_update_hdr_metadata() doxy should in any case point that the frame or at least its side data must be writable to prevent other uses in the future where this may not be taken into account, now that it's shared. And an actual, generic solution like a make_writable function is imo still a good idea, unless the user is expected to use av_frame_make_writable() if they want to do *any* kind of in-place change to the frame.
diff --git a/libavfilter/colorspace.c b/libavfilter/colorspace.c index d6f6055401..c6682216d6 100644 --- a/libavfilter/colorspace.c +++ b/libavfilter/colorspace.c @@ -118,3 +118,20 @@ double ff_determine_signal_peak(AVFrame *in) return peak; } + +void ff_update_hdr_metadata(AVFrame *in, double peak) +{ + AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); + + if (sd) { + AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data; + clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE); + } + + sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); + if (sd) { + AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data; + if (metadata->has_luminance) + metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000); + } +} diff --git a/libavfilter/colorspace.h b/libavfilter/colorspace.h index 75705ecfc7..936681815a 100644 --- a/libavfilter/colorspace.h +++ b/libavfilter/colorspace.h @@ -45,5 +45,6 @@ void ff_fill_rgb2xyz_table(const struct PrimaryCoefficients *coeffs, double rgb2xyz[3][3]); double ff_determine_signal_peak(AVFrame *in); +void ff_update_hdr_metadata(AVFrame *in, double peak); #endif diff --git a/libavfilter/vf_tonemap_opencl.c b/libavfilter/vf_tonemap_opencl.c index 7455ebb9fb..5da2333169 100644 --- a/libavfilter/vf_tonemap_opencl.c +++ b/libavfilter/vf_tonemap_opencl.c @@ -21,7 +21,6 @@ #include "libavutil/bprint.h" #include "libavutil/common.h" #include "libavutil/imgutils.h" -#include "libavutil/mastering_display_metadata.h" #include "libavutil/mem.h" #include "libavutil/opt.h" #include "libavutil/pixdesc.h" @@ -342,22 +341,6 @@ fail: return err; } -static void update_metadata(AVFrame *in, double peak) { - AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); - - if (sd) { - AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data; - clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE); - } - - sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); - if (sd) { - AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data; - if (metadata->has_luminance) - metadata->max_luminance =av_d2q(peak * REFERENCE_WHITE, 10000); - } -} - static int tonemap_opencl_filter_frame(AVFilterLink *inlink, AVFrame *input) { AVFilterContext *avctx = inlink->dst; @@ -444,7 +427,7 @@ static int tonemap_opencl_filter_frame(AVFilterLink *inlink, AVFrame *input) av_frame_free(&input); - update_metadata(output, ctx->target_peak); + ff_update_hdr_metadata(output, ctx->target_peak); av_log(ctx, AV_LOG_DEBUG, "Tone-mapping output: %s, %ux%u (%"PRId64").\n", av_get_pix_fmt_name(output->format),