Re: [FFmpeg-devel] [PATCH 03/25] avfilter: add negotiation API for color space/range

[Anton Khirnov <anton@khirnov.net>]

Quoting Niklas Haas (2023-11-09 13:19:35)
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 12383a28d3..ce3f90a674 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,14 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-11-xx - xxxxxxxxxx - lavf 58.14.100 - avfilter.h formats.h
> +  Add AVFilterFormatsConfig.color_spaces, AVFilterFormatsConfig.color_ranges,
> +  AVFilterLink.colorspace, AVFilterLink.color_range, ff_all_color_spaces,
> +  ff_all_color_ranges, ff_set_common_color_spaces, ff_set_common_color_ranges,
> +  ff_set_common_color_spaces_from_list, ff_set_common_color_ranges_from_list,
> +  ff_set_common_all_color_spaces, ff_set_common_all_color_ranges,
> +  ff_formats_check_color_spaces, ff_formats_check_color_ranges.

ff* are private and so shouldn't be mentioned in APIchanges

AVFilterFormatsConfig lives in a public header, but seems not to be
usable by API users.

> +
>  2023-11-08 - xxxxxxxxxx - lavu 58.32.100 - channel_layout.h
>    Add AV_CH_LAYOUT_7POINT2POINT3 and AV_CHANNEL_LAYOUT_7POINT2POINT3.
>    Add AV_CH_LAYOUT_9POINT1POINT4_BACK and AV_CHANNEL_LAYOUT_9POINT1POINT4_BACK.
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index ab7782862a..77bfec00c5 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -185,6 +185,7 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
>      link->type    = src->output_pads[srcpad].type;
>      av_assert0(AV_PIX_FMT_NONE == -1 && AV_SAMPLE_FMT_NONE == -1);
>      link->format  = -1;
> +    link->colorspace = AVCOL_SPC_UNSPECIFIED;
>      ff_framequeue_init(&link->fifo, &src->graph->internal->frame_queues);
>  
>      return 0;
> @@ -286,6 +287,12 @@ int avfilter_insert_filter(AVFilterLink *link, AVFilterContext *filt,
>      if (link->outcfg.formats)
>          ff_formats_changeref(&link->outcfg.formats,
>                               &filt->outputs[filt_dstpad_idx]->outcfg.formats);
> +    if (link->outcfg.color_spaces)
> +        ff_formats_changeref(&link->outcfg.color_spaces,
> +                             &filt->outputs[filt_dstpad_idx]->outcfg.color_spaces);
> +    if (link->outcfg.color_ranges)
> +        ff_formats_changeref(&link->outcfg.color_ranges,
> +                             &filt->outputs[filt_dstpad_idx]->outcfg.color_ranges);
>      if (link->outcfg.samplerates)
>          ff_formats_changeref(&link->outcfg.samplerates,
>                               &filt->outputs[filt_dstpad_idx]->outcfg.samplerates);
> @@ -730,6 +737,10 @@ static void free_link(AVFilterLink *link)
>  
>      ff_formats_unref(&link->incfg.formats);
>      ff_formats_unref(&link->outcfg.formats);
> +    ff_formats_unref(&link->incfg.color_spaces);
> +    ff_formats_unref(&link->outcfg.color_spaces);
> +    ff_formats_unref(&link->incfg.color_ranges);
> +    ff_formats_unref(&link->outcfg.color_ranges);
>      ff_formats_unref(&link->incfg.samplerates);
>      ff_formats_unref(&link->outcfg.samplerates);
>      ff_channel_layouts_unref(&link->incfg.channel_layouts);
> @@ -987,9 +998,11 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame)
>              strcmp(link->dst->filter->name, "idet") &&
>              strcmp(link->dst->filter->name, "null") &&
>              strcmp(link->dst->filter->name, "scale")) {
> -            av_assert1(frame->format                 == link->format);
> -            av_assert1(frame->width               == link->w);
> -            av_assert1(frame->height               == link->h);
> +            av_assert1(frame->format        == link->format);
> +            av_assert1(frame->width         == link->w);
> +            av_assert1(frame->height        == link->h);
> +            av_assert1(frame->colorspace    == link->color_space);
                                                             ^
Should not be there.

Also, these fail a LOT with this patch. Most of them seem fixed by later
patches, so it's probably better to move them to the end of the set?

> @@ -583,6 +602,29 @@ static enum AVSampleFormat find_best_sample_fmt_of_2(enum AVSampleFormat dst_fmt
>      return score1 < score2 ? dst_fmt1 : dst_fmt2;
>  }
>  
> +int ff_fmt_is_regular_yuv(enum AVPixelFormat fmt)
> +{
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(fmt);
> +    if (!desc)
> +        return 0;
> +    if (desc->nb_components < 3)
> +        return 0; /* Grayscale is explicitly full-range in swscale */
> +    if (desc->flags & (AV_PIX_FMT_FLAG_RGB | AV_PIX_FMT_FLAG_PAL |
> +                       AV_PIX_FMT_FLAG_XYZ | AV_PIX_FMT_FLAG_FLOAT))

Should this include AV_PIX_FMT_FLAG_HWACCEL too?

> +        return 0;
> +
> +    switch (fmt) {
> +    case AV_PIX_FMT_YUVJ420P:
> +    case AV_PIX_FMT_YUVJ422P:
> +    case AV_PIX_FMT_YUVJ444P:
> +    case AV_PIX_FMT_YUVJ440P:
> +    case AV_PIX_FMT_YUVJ411P:
> +        return 0;
> +    default:
> +        return 1;
> +    }
> +}
> +
>  static int pick_format(AVFilterLink *link, AVFilterLink *ref)
>  {
>      if (!link || !link->incfg.formats)
> @@ -603,6 +645,7 @@ static int pick_format(AVFilterLink *link, AVFilterLink *ref)
>                     av_get_pix_fmt_name(ref->format), has_alpha);
>              link->incfg.formats->formats[0] = best;
>          }
> +

unrelated

-- 
Anton Khirnov
_______________________________________________
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".