diff mbox

[FFmpeg-devel,08/25] fftools/ffplay: support only limited color range

Message ID 20171216101245.26977-8-onemda@gmail.com
State Superseded
Headers show

Commit Message

Paul B Mahol Dec. 16, 2017, 10:12 a.m. UTC 
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 fftools/ffplay.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Marton Balint Dec. 16, 2017, 11:24 a.m. UTC | #1 
On Sat, 16 Dec 2017, Paul B Mahol wrote:

> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
> fftools/ffplay.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 10a917194d..f023c81575 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -1822,6 +1822,7 @@ fail:
> static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const char *vfilters, AVFrame *frame)
> {
>     enum AVPixelFormat pix_fmts[FF_ARRAY_ELEMS(sdl_texture_format_map)];
> +    enum AVColorRange color_ranges[2] = { AVCOL_RANGE_MPEG, AVCOL_RANGE_UNSPECIFIED };
>     char sws_flags_str[512] = "";
>     char buffersrc_args[256];
>     int ret;
> @@ -1876,7 +1877,10 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>     if ((ret = av_opt_set_int_list(filt_out, "pix_fmts", pix_fmts,  AV_PIX_FMT_NONE, AV_OPT_SEARCH_CHILDREN)) < 0)
>         goto fail;
> 
> -    last_filter = filt_out;
> +    if ((ret = av_opt_set_int_list(filt_out, "color_ranges", color_ranges, AVCOL_RANGE_UNSPECIFIED, AV_OPT_SEARCH_CHILDREN)) < 0)
> +        goto fail;
> +
> +     last_filter = filt_out;

I am afraid this wont work, because ffplay supports full range RGB as 
well. Unless there is a way to specify allowed pixel format / color range 
combinations (which is the only way to mimic existing behaviour as far as 
I see), you have to use hacks like configure the filter graph once without 
a range restriction, and if you get an invalid pixel format / color range 
combination, you have to configure the graph again with the supported 
color range list for that pixel format, hoping that the pixel format will 
remain the same.

Regards,
Marton
Paul B Mahol Dec. 16, 2017, 1:44 p.m. UTC | #2 
On 12/16/17, Marton Balint <cus@passwd.hu> wrote:
>
>
> On Sat, 16 Dec 2017, Paul B Mahol wrote:
>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>> fftools/ffplay.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>> index 10a917194d..f023c81575 100644
>> --- a/fftools/ffplay.c
>> +++ b/fftools/ffplay.c
>> @@ -1822,6 +1822,7 @@ fail:
>> static int configure_video_filters(AVFilterGraph *graph, VideoState *is,
>> const char *vfilters, AVFrame *frame)
>> {
>>     enum AVPixelFormat pix_fmts[FF_ARRAY_ELEMS(sdl_texture_format_map)];
>> +    enum AVColorRange color_ranges[2] = { AVCOL_RANGE_MPEG,
>> AVCOL_RANGE_UNSPECIFIED };
>>     char sws_flags_str[512] = "";
>>     char buffersrc_args[256];
>>     int ret;
>> @@ -1876,7 +1877,10 @@ static int configure_video_filters(AVFilterGraph
>> *graph, VideoState *is, const c
>>     if ((ret = av_opt_set_int_list(filt_out, "pix_fmts", pix_fmts,
>> AV_PIX_FMT_NONE, AV_OPT_SEARCH_CHILDREN)) < 0)
>>         goto fail;
>>
>> -    last_filter = filt_out;
>> +    if ((ret = av_opt_set_int_list(filt_out, "color_ranges",
>> color_ranges, AVCOL_RANGE_UNSPECIFIED, AV_OPT_SEARCH_CHILDREN)) < 0)
>> +        goto fail;
>> +
>> +     last_filter = filt_out;
>
> I am afraid this wont work, because ffplay supports full range RGB as
> well. Unless there is a way to specify allowed pixel format / color range
> combinations (which is the only way to mimic existing behaviour as far as
> I see), you have to use hacks like configure the filter graph once without
> a range restriction, and if you get an invalid pixel format / color range
> combination, you have to configure the graph again with the supported
> color range list for that pixel format, hoping that the pixel format will
> remain the same.

If you actually checked output, it will appear same as before patchset.
Nicolas George Dec. 16, 2017, 5:29 p.m. UTC | #3 
Marton Balint (2017-12-16):
> I am afraid this wont work, because ffplay supports full range RGB as well.
> Unless there is a way to specify allowed pixel format / color range
> combinations (which is the only way to mimic existing behaviour as far as I
> see), you have to use hacks like configure the filter graph once without a
> range restriction, and if you get an invalid pixel format / color range
> combination, you have to configure the graph again with the supported color
> range list for that pixel format, hoping that the pixel format will remain
> the same.

It seems you are confirming something that I suspected and needed to
check, i.e. that negotiating range separately from pixel format (or at
least RGB / YUV) will not give satisfactory results. I fear a combined
negotiation will be necessary.

Regards,
Paul B Mahol Dec. 16, 2017, 5:36 p.m. UTC | #4 
On 12/16/17, Nicolas George <george@nsup.org> wrote:
> Marton Balint (2017-12-16):
>> I am afraid this wont work, because ffplay supports full range RGB as
>> well.
>> Unless there is a way to specify allowed pixel format / color range
>> combinations (which is the only way to mimic existing behaviour as far as
>> I
>> see), you have to use hacks like configure the filter graph once without a
>> range restriction, and if you get an invalid pixel format / color range
>> combination, you have to configure the graph again with the supported
>> color
>> range list for that pixel format, hoping that the pixel format will remain
>> the same.
>
> It seems you are confirming something that I suspected and needed to
> check, i.e. that negotiating range separately from pixel format (or at
> least RGB / YUV) will not give satisfactory results. I fear a combined
> negotiation will be necessary.

Not really. This is just ffplay nuisance.
Nicolas George Dec. 17, 2017, 12:03 p.m. UTC | #5 
Paul B Mahol (2017-12-16):
> Not really. This is just ffplay nuisance.

Marton and I do not think so. Your patches, your burden of the proof.
Rejected as is.
wm4 Dec. 17, 2017, 12:55 p.m. UTC | #6 
On Sat, 16 Dec 2017 11:12:28 +0100
Paul B Mahol <onemda@gmail.com> wrote:

> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  fftools/ffplay.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 10a917194d..f023c81575 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -1822,6 +1822,7 @@ fail:
>  static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const char *vfilters, AVFrame *frame)
>  {
>      enum AVPixelFormat pix_fmts[FF_ARRAY_ELEMS(sdl_texture_format_map)];
> +    enum AVColorRange color_ranges[2] = { AVCOL_RANGE_MPEG, AVCOL_RANGE_UNSPECIFIED };
>      char sws_flags_str[512] = "";
>      char buffersrc_args[256];
>      int ret;
> @@ -1876,7 +1877,10 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>      if ((ret = av_opt_set_int_list(filt_out, "pix_fmts", pix_fmts,  AV_PIX_FMT_NONE, AV_OPT_SEARCH_CHILDREN)) < 0)
>          goto fail;
>  
> -    last_filter = filt_out;
> +    if ((ret = av_opt_set_int_list(filt_out, "color_ranges", color_ranges, AVCOL_RANGE_UNSPECIFIED, AV_OPT_SEARCH_CHILDREN)) < 0)
> +        goto fail;
> +
> +     last_filter = filt_out;
>  
>  /* Note: this macro adds a filter before the lastly added filter, so the
>   * processing order of the filters is in reverse */

LGTM
Marton Balint Dec. 17, 2017, 6:43 p.m. UTC | #7 
On Sat, 16 Dec 2017, Paul B Mahol wrote:

> On 12/16/17, Marton Balint <cus@passwd.hu> wrote:
>>
>>
>> On Sat, 16 Dec 2017, Paul B Mahol wrote:
>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>> fftools/ffplay.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>>> index 10a917194d..f023c81575 100644
>>> --- a/fftools/ffplay.c
>>> +++ b/fftools/ffplay.c
>>> @@ -1822,6 +1822,7 @@ fail:
>>> static int configure_video_filters(AVFilterGraph *graph, VideoState *is,
>>> const char *vfilters, AVFrame *frame)
>>> {
>>>     enum AVPixelFormat pix_fmts[FF_ARRAY_ELEMS(sdl_texture_format_map)];
>>> +    enum AVColorRange color_ranges[2] = { AVCOL_RANGE_MPEG,
>>> AVCOL_RANGE_UNSPECIFIED };
>>>     char sws_flags_str[512] = "";
>>>     char buffersrc_args[256];
>>>     int ret;
>>> @@ -1876,7 +1877,10 @@ static int configure_video_filters(AVFilterGraph
>>> *graph, VideoState *is, const c
>>>     if ((ret = av_opt_set_int_list(filt_out, "pix_fmts", pix_fmts,
>>> AV_PIX_FMT_NONE, AV_OPT_SEARCH_CHILDREN)) < 0)
>>>         goto fail;
>>>
>>> -    last_filter = filt_out;
>>> +    if ((ret = av_opt_set_int_list(filt_out, "color_ranges",
>>> color_ranges, AVCOL_RANGE_UNSPECIFIED, AV_OPT_SEARCH_CHILDREN)) < 0)
>>> +        goto fail;
>>> +
>>> +     last_filter = filt_out;
>>
>> I am afraid this wont work, because ffplay supports full range RGB as
>> well. Unless there is a way to specify allowed pixel format / color range
>> combinations (which is the only way to mimic existing behaviour as far as
>> I see), you have to use hacks like configure the filter graph once without
>> a range restriction, and if you get an invalid pixel format / color range
>> combination, you have to configure the graph again with the supported
>> color range list for that pixel format, hoping that the pixel format will
>> remain the same.
>
> If you actually checked output, it will appear same as before patchset.

If this works, that means color_ranges are ignored for RGB. We can do 
that, but please document every color_range field explicitly that they 
only affect YUV, nothing else.

Also there is a stray whitespace addition in the last line:
" last_filter = filt_out"

Regards,
Marton
Nicolas George Dec. 17, 2017, 8:03 p.m. UTC | #8 
Marton Balint (2017-12-17):
> If this works, that means color_ranges are ignored for RGB. We can do that,
> but please document every color_range field explicitly that they only affect
> YUV, nothing else.

This is very ugly. I can accept that only if several persons who know
about colorspaces and the trends in the industry confirm that it will
not come back to bite us later, i.e. that only YUV pixel formats will
ever need that kind of information.

Regards,
diff mbox

Patch

diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index 10a917194d..f023c81575 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -1822,6 +1822,7 @@  fail:
 static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const char *vfilters, AVFrame *frame)
 {
     enum AVPixelFormat pix_fmts[FF_ARRAY_ELEMS(sdl_texture_format_map)];
+    enum AVColorRange color_ranges[2] = { AVCOL_RANGE_MPEG, AVCOL_RANGE_UNSPECIFIED };
     char sws_flags_str[512] = "";
     char buffersrc_args[256];
     int ret;
@@ -1876,7 +1877,10 @@  static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
     if ((ret = av_opt_set_int_list(filt_out, "pix_fmts", pix_fmts,  AV_PIX_FMT_NONE, AV_OPT_SEARCH_CHILDREN)) < 0)
         goto fail;
 
-    last_filter = filt_out;
+    if ((ret = av_opt_set_int_list(filt_out, "color_ranges", color_ranges, AVCOL_RANGE_UNSPECIFIED, AV_OPT_SEARCH_CHILDREN)) < 0)
+        goto fail;
+
+     last_filter = filt_out;
 
 /* Note: this macro adds a filter before the lastly added filter, so the
  * processing order of the filters is in reverse */