Re: [FFmpeg-devel] [PATCH v5] avcodec/h2645_sei: validate Mastering Display Colour Volume SEI values

[Niklas Haas <ffmpeg@haasn.xyz>]

On Sat, 13 Apr 2024 17:21:08 +0200 Kacper Michajłow <kasper93@gmail.com> wrote:
> As we can read in ST 2086:
> 
> Values outside the specified ranges of luminance and chromaticity values
> are not reserved by SMPTE, and can be used for purposes outside the
> scope of this standard.
> 
> This is further acknowledged by ITU-T H.264 and ITU-T H.265. Which says
> that values out of range are unknown or unspecified or specified by
> other means not specified in this Specification.
> 
> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> ---
>  libavcodec/h2645_sei.c | 55 ++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/h2645_sei.c b/libavcodec/h2645_sei.c
> index 933975f076..96a22e7cf6 100644
> --- a/libavcodec/h2645_sei.c
> +++ b/libavcodec/h2645_sei.c
> @@ -587,38 +587,61 @@ static int h2645_sei_to_side_data(AVCodecContext *avctx, H2645SEI *sei,
>              return ret;
>  
>          if (metadata) {
> +            metadata->has_luminance = 1;
> +            metadata->has_primaries = 1;
> +
>              for (i = 0; i < 3; i++) {
>                  const int j = mapping[i];
>                  metadata->display_primaries[i][0].num = sei->mastering_display.display_primaries[j][0];
>                  metadata->display_primaries[i][0].den = chroma_den;
> +                metadata->has_primaries &= sei->mastering_display.display_primaries[j][0] >= 5 &&
> +                                           sei->mastering_display.display_primaries[j][0] <= 37000;
> +
>                  metadata->display_primaries[i][1].num = sei->mastering_display.display_primaries[j][1];
>                  metadata->display_primaries[i][1].den = chroma_den;
> +                metadata->has_primaries &= sei->mastering_display.display_primaries[j][1] >= 5 &&
> +                                           sei->mastering_display.display_primaries[j][1] <= 42000;
>              }
>              metadata->white_point[0].num = sei->mastering_display.white_point[0];
>              metadata->white_point[0].den = chroma_den;
> +            metadata->has_primaries &= sei->mastering_display.white_point[0] >= 5 &&
> +                                       sei->mastering_display.white_point[0] <= 37000;
> +
>              metadata->white_point[1].num = sei->mastering_display.white_point[1];
>              metadata->white_point[1].den = chroma_den;
> +            metadata->has_primaries &= sei->mastering_display.white_point[1] >= 5 &&
> +                                       sei->mastering_display.white_point[1] <= 42000;
>  
>              metadata->max_luminance.num = sei->mastering_display.max_luminance;
>              metadata->max_luminance.den = luma_den;
> +            metadata->has_luminance &= sei->mastering_display.max_luminance >= 50000 &&
> +                                       sei->mastering_display.max_luminance <= 100000000;
> +
>              metadata->min_luminance.num = sei->mastering_display.min_luminance;
>              metadata->min_luminance.den = luma_den;
> -            metadata->has_luminance = 1;
> -            metadata->has_primaries = 1;
> -
> -            av_log(avctx, AV_LOG_DEBUG, "Mastering Display Metadata:\n");
> -            av_log(avctx, AV_LOG_DEBUG,
> -                   "r(%5.4f,%5.4f) g(%5.4f,%5.4f) b(%5.4f %5.4f) wp(%5.4f, %5.4f)\n",
> -                   av_q2d(metadata->display_primaries[0][0]),
> -                   av_q2d(metadata->display_primaries[0][1]),
> -                   av_q2d(metadata->display_primaries[1][0]),
> -                   av_q2d(metadata->display_primaries[1][1]),
> -                   av_q2d(metadata->display_primaries[2][0]),
> -                   av_q2d(metadata->display_primaries[2][1]),
> -                   av_q2d(metadata->white_point[0]), av_q2d(metadata->white_point[1]));
> -            av_log(avctx, AV_LOG_DEBUG,
> -                   "min_luminance=%f, max_luminance=%f\n",
> -                   av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
> +            metadata->has_luminance &= sei->mastering_display.min_luminance >= 1 &&
> +                                       sei->mastering_display.min_luminance <= 50000 &&
> +                                       sei->mastering_display.min_luminance <
> +                                       sei->mastering_display.max_luminance;
> +
> +            if (metadata->has_luminance || metadata->has_primaries)
> +                av_log(avctx, AV_LOG_DEBUG, "Mastering Display Metadata:\n");
> +            if (metadata->has_primaries) {
> +                av_log(avctx, AV_LOG_DEBUG,
> +                       "r(%5.4f,%5.4f) g(%5.4f,%5.4f) b(%5.4f %5.4f) wp(%5.4f, %5.4f)\n",
> +                       av_q2d(metadata->display_primaries[0][0]),
> +                       av_q2d(metadata->display_primaries[0][1]),
> +                       av_q2d(metadata->display_primaries[1][0]),
> +                       av_q2d(metadata->display_primaries[1][1]),
> +                       av_q2d(metadata->display_primaries[2][0]),
> +                       av_q2d(metadata->display_primaries[2][1]),
> +                       av_q2d(metadata->white_point[0]), av_q2d(metadata->white_point[1]));
> +            }
> +            if (metadata->has_luminance) {
> +                av_log(avctx, AV_LOG_DEBUG,
> +                       "min_luminance=%f, max_luminance=%f\n",
> +                       av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
> +            }
>          }
>      }
>  
> -- 
> 2.43.0
> 
> _______________________________________________
> 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".

LGTM.

As was discussed on IRC, this validation logic strictly follows the
wording of SMPTE ST 2086 and H.264/H.265. While we may wish to handle
invalid metadata in a different layer, there is no similar bitstream
conformance requirements in AV1 etc. (And who's to say what brightness
ranges for the min/max *should* be considered "valid"?)

Merged as 1c45104f74e59beb25ce7451c98527f7a3c518f6
_______________________________________________
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".