[FFmpeg-devel] [PATCH] ffprobe: Print color properties from show_frames

[Tobias Rapp]

On 20.07.2017 16:19, Vittorio Giovara wrote:
>> On 20.07.2017 14:38, Vittorio Giovara wrote:
>>> ---
>>>  ffprobe.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/ffprobe.c b/ffprobe.c
>>> index f6d9be0df9..412e2dadab 100644
>>> --- a/ffprobe.c
>>> +++ b/ffprobe.c
>>> @@ -2105,6 +2105,12 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream,
>>>          print_int("interlaced_frame",       frame->interlaced_frame);
>>>          print_int("top_field_first",        frame->top_field_first);
>>>          print_int("repeat_pict",            frame->repeat_pict);
>>> +
>>> +        print_str("color_range",     av_color_range_name(frame->color_range));
>>> +        print_str("color_space",     av_color_space_name(frame->colorspace));
>>> +        print_str("color_primaries", av_color_primaries_name(frame->color_primaries));
>>> +        print_str("color_transfer",  av_color_transfer_name(frame->color_trc));
>>> +        print_str("chroma_location", av_chroma_location_name(frame->chroma_location));
>>
>> I guess this should look like
>>
>> if (frame->... != ..._UNSPECIFIED)
>>     print_str(...);
>> else
>>     print_str_opt(...);
>>
>> see the similar code lines handling color properties on stream level (~
>> line #2475).
>
> Should it? That approach effectively hides these parameters from the
> output if unknown, and I often want to know as much as possible when
> hunting down parameters with read_frames (even that the filed is just
> "unknown" and not missing). Also if these fields are always output, it
> simplify parsing them quite a bit, don't you think so? I'd much rather
> change the stream level code to output more information instead.
>

It depends on the writer whether print_str_opt actually writes to the 
output or not. For example when using CSV optional data is always 
written, when using XML it is skipped.

In my humble opinion for the XML output format it would only increase 
data size and make the parser slightly _more_ complex in case optional 
fields would be written (as the reader would have to check if the field 
value equals "unknown" or "N/A" strings to handle the value-not-valid 
situation). So I see no benefit in replacing print_str_opt with 
print_str globally.

>>>          break;
>>>
>>>      case AVMEDIA_TYPE_AUDIO:
>>>
>>
>> The schema file at doc/ffprobe.xsd should be updated to reflect the new
>> fields.
>>
>> Also I assume that some FATE references are changed by this patch?
>
> Right, I'll update them in the next iteration, thanks for noticing.
>

Regards,
Tobias