Re: [FFmpeg-devel] [PATCH 3/4] vf_tonemap_opencl: Move update_metadata() to a shared file
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,
>>> 1);
>>> +}
>> 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] vf_tonemap_opencl: Move update_metadata() to a shared file
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, 1);
>> +}
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] vf_tonemap_opencl: Move update_metadata() to a shared file
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, 1);
> +}
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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/4] vf_tonemap_opencl: Move update_metadata() to a shared file
---
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, 1);
+}
+}
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, 1);
-}
-}
-
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();
-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),
--
2.17.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
