Re: [FFmpeg-devel] [PATCH v4 5/7] lavfi/vaapi: Improve support for colour properties

[Michael Niedermayer]

On Tue, Apr 09, 2019 at 11:07:28PM +0100, Mark Thompson wrote:
> Attempts to pick the set of supported colour properties best matching the
> input.  Output is then set with the same values, except for the colour
> matrix which may change when converting between RGB and YUV.
> ---
> On 04/03/2019 11:36, Song, Ruiling wrote:
> > Why using these magic number instead of meaningful enum value?
> 
> See .  
> These values are standardised in H.273 (or ISO/IEC 23001-8:2016), which are 
> then used directly in both FFmpeg and various codecs like MPEG-2, H.264, 
> H.265 and AV1.  On balance it seemed clearer to keep the numeric values 
> rather than to turn them into symbolic names which would be much harder to 
> read.
> 
> > And two entries added for VAProcColorStandardBT601, seems that only the 
> > first will be returned?
> 
> It was meant to check both cases, now fixed.
> 
> >> +// Give scores to the possible options and choose the lowest one.
> >> +// An exact match will score zero and therefore always be chosen, as
> >> +// will a partial match where all unmatched elements are explicitly
> >> +// unspecified.  (If all elements are unspecified this will use the
> >> +// first available value.)  If no options match at all then just
> >> +// pass "none" to the driver and let it make its own choice.
> > Here (a*4+b*2+c)  is chosen as the score function, I am not sure whether (a 
> > + b + c) is just ok?
> 
> I made the weights different to try to avoid confusing draws where it might 
> match to a different colourspace on one run and then a different transfer 
> characteristic on another.
> 
> The choice of matrix > transfer > primaries is mostly arbitrary coming from 
> vague intuition about how extreme the bad cases are (wrong matrix can be 
> wildly incorrect (YCoCg interpreted as YUV, say), wrong transfer can mess up 
> the intensity in a confusing way but should mostly resemble the intended 
> image, wrong primaries will shift colours and look weird but stay be mostly 
> coherent).  I'm happy to change that if you have a different opinion!
> 
> >> +best_score = -1;
> >> +worst_score = 4 * (props->colorspace != AVCOL_SPC_UNSPECIFIED) +
> >> +  2 * (props->color_trc != AVCOL_TRC_UNSPECIFIED) +
> >> +  (props->color_primaries != AVCOL_PRI_UNSPECIFIED);
> > Seems that the outer loop here is just used to re-iterate through nb_vacs 
> > to find the best match again?
> > Can we remove the outer-loop-over-k like below?
> > 
> > best_va_standard = VAProcColorStandardNone;
> > for (i = 0; i < nb_vacs; i++) {
> > ...
> > ...
> >// Only include things which matched something.
> > if ((worst_score == 0 || score < worst_score) &&
> > (best_score == -1 || score < best_score)) {
> > best_score = score;
> > best_va_standard = t->va_color_standard;
> > }
> > }
> > props->va_color_standard = best_va_standard;
> 
> Yes, that approach would be nicer.  Done.
> 
> Thanks,
> 
> - Mark
> 
> 
>  libavfilter/vaapi_vpp.c| 285 +++--
>  libavfilter/vaapi_vpp.h|   2 -
>  libavfilter/vf_deinterlace_vaapi.c |   8 +-
>  libavfilter/vf_misc_vaapi.c|   7 +-
>  libavfilter/vf_procamp_vaapi.c |   7 +-
>  libavfilter/vf_scale_vaapi.c   |   8 +-
>  libavfilter/vf_transpose_vaapi.c   |   7 +-
>  7 files changed, 292 insertions(+), 32 deletions(-)
> 
> diff --git a/libavfilter/vaapi_vpp.c b/libavfilter/vaapi_vpp.c
> index 647ddc0811..5091d2508a 100644
> --- a/libavfilter/vaapi_vpp.c
> +++ b/libavfilter/vaapi_vpp.c

breaks build 
CC  libavfilter/vaapi_vpp.o
libavfilter/vaapi_vpp.c: In function ‘vaapi_vpp_colour_properties’:
libavfilter/vaapi_vpp.c:466:43: error: ‘VAProcColorStandardExplicit’ undeclared 
(first use in this function)
 if (output_props.va_color_standard != VAProcColorStandardExplicit) {
   ^
libavfilter/vaapi_vpp.c:466:43: note: each undeclared identifier is reported 
only once for each function it appears in
make: *** [libavfilter/vaapi_vpp.o] Error 1
make: Target `all' not remade because of errors.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v4 5/7] lavfi/vaapi: Improve support for colour properties

[Mark Thompson]

Attempts to pick the set of supported colour properties best matching the
input.  Output is then set with the same values, except for the colour
matrix which may change when converting between RGB and YUV.
---
On 04/03/2019 11:36, Song, Ruiling wrote:
> Why using these magic number instead of meaningful enum value?

See .  These 
values are standardised in H.273 (or ISO/IEC 23001-8:2016), which are then used 
directly in both FFmpeg and various codecs like MPEG-2, H.264, H.265 and AV1.  
On balance it seemed clearer to keep the numeric values rather than to turn 
them into symbolic names which would be much harder to read.

> And two entries added for VAProcColorStandardBT601, seems that only the first 
> will be returned?

It was meant to check both cases, now fixed.

>> +// Give scores to the possible options and choose the lowest one.
>> +// An exact match will score zero and therefore always be chosen, as
>> +// will a partial match where all unmatched elements are explicitly
>> +// unspecified.  (If all elements are unspecified this will use the
>> +// first available value.)  If no options match at all then just
>> +// pass "none" to the driver and let it make its own choice.
> Here (a*4+b*2+c)  is chosen as the score function, I am not sure whether (a + 
> b + c) is just ok?

I made the weights different to try to avoid confusing draws where it might 
match to a different colourspace on one run and then a different transfer 
characteristic on another.

The choice of matrix > transfer > primaries is mostly arbitrary coming from 
vague intuition about how extreme the bad cases are (wrong matrix can be wildly 
incorrect (YCoCg interpreted as YUV, say), wrong transfer can mess up the 
intensity in a confusing way but should mostly resemble the intended image, 
wrong primaries will shift colours and look weird but stay be mostly coherent). 
 I'm happy to change that if you have a different opinion!

>> +best_score = -1;
>> +worst_score = 4 * (props->colorspace != AVCOL_SPC_UNSPECIFIED) +
>> +  2 * (props->color_trc != AVCOL_TRC_UNSPECIFIED) +
>> +  (props->color_primaries != AVCOL_PRI_UNSPECIFIED);
> Seems that the outer loop here is just used to re-iterate through nb_vacs to 
> find the best match again?
> Can we remove the outer-loop-over-k like below?
> 
> best_va_standard = VAProcColorStandardNone;
> for (i = 0; i < nb_vacs; i++) {
>   ...
>   ...
>// Only include things which matched something.
> if ((worst_score == 0 || score < worst_score) &&
> (best_score == -1 || score < best_score)) {
> best_score = score;
>   best_va_standard = t->va_color_standard;
>   }
> }
> props->va_color_standard = best_va_standard;

Yes, that approach would be nicer.  Done.

Thanks,

- Mark


 libavfilter/vaapi_vpp.c| 285 +++--
 libavfilter/vaapi_vpp.h|   2 -
 libavfilter/vf_deinterlace_vaapi.c |   8 +-
 libavfilter/vf_misc_vaapi.c|   7 +-
 libavfilter/vf_procamp_vaapi.c |   7 +-
 libavfilter/vf_scale_vaapi.c   |   8 +-
 libavfilter/vf_transpose_vaapi.c   |   7 +-
 7 files changed, 292 insertions(+), 32 deletions(-)

diff --git a/libavfilter/vaapi_vpp.c b/libavfilter/vaapi_vpp.c
index 647ddc0811..5091d2508a 100644
--- a/libavfilter/vaapi_vpp.c
+++ b/libavfilter/vaapi_vpp.c
@@ -234,18 +234,275 @@ fail:
 return err;
 }
 
-int ff_vaapi_vpp_colour_standard(enum AVColorSpace av_cs)
+typedef struct VAAPIColourProperties {
+VAProcColorStandardType va_color_standard;
+
+enum AVColorPrimaries color_primaries;
+enum AVColorTransferCharacteristic color_trc;
+enum AVColorSpace colorspace;
+
+uint8_t va_chroma_sample_location;
+uint8_t va_color_range;
+
+enum AVColorRange color_range;
+enum AVChromaLocation chroma_sample_location;
+} VAAPIColourProperties;
+
+static const VAAPIColourProperties vaapi_colour_standard_map[] = {
+{ VAProcColorStandardBT601,   5,  6,  5 },
+{ VAProcColorStandardBT601,   6,  6,  6 },
+{ VAProcColorStandardBT709,   1,  1,  1 },
+{ VAProcColorStandardBT470M,  4,  4,  4 },
+{ VAProcColorStandardBT470BG, 5,  5,  5 },
+{ VAProcColorStandardSMPTE170M,   6,  6,  6 },
+{ VAProcColorStandardSMPTE240M,   7,  7,  7 },
+{ VAProcColorStandardGenericFilm, 8,  1,  1 },
+#if VA_CHECK_VERSION(1, 1, 0)
+{ VAProcColorStandardSRGB,1, 13,  0 },
+{ VAProcColorStandardXVYCC601,1, 11,  5 },
+{ VAProcColorStandardXVYCC709,1, 11,  1 },
+{ VAProcColorStandardBT2020,  9, 14,  9 },
+#endif
+};
+
+static void vaapi_vpp_fill_colour_standard(VAAPIColourProperties *props,
+   VAProcColorStandardType *vacs,
+   int nb_vacs)
+{
+const VAAPIColourPrope