[FFmpeg-devel] [PATCH] ffprobe: fix sample aspect ratio output when sample aspect ratio is not set

[Robert Krüger]

On 23.04.2010, at 01:37, Stefano Sabatini wrote:

> On date Wednesday 2010-04-21 02:11:16 +0200, Stefano Sabatini encoded:
>> On date Tuesday 2010-04-20 10:02:52 +0200, Robert Kr?ger encoded:
>>> 
>>> On 19.04.2010, at 23:39, Stefano Sabatini wrote:
>>> 
>>>> On date Friday 2010-04-16 18:40:53 +0200, Robert Kr?ger encoded:
> [...]
>>>>> You're probably right although I would suspect that there are tons
>>>>> of programs out there which parse the output of ffmpeg to determine
>>>>> the display size of the video which will have an incorrect result
>>>>> here but that's beside the point, I know. What I would like someone
>>>>> to decide is how ffprobe should communicate the sample aspect ratio
>>>>> (and consequently the display aspect ratio as well) not being
>>>>> available (i.e. numerator being zero, at least that criterion is
>>>>> used in other places in ffmpeg code)
>>>>> 
>>>>> 1) output "N/A" for both values
>>>>> 2) don't output those key value pairs at all
>>>>> 3) output what's in the struct (e.g. 0:1 in my case) for both like it does now
>>>>> 
>>>>> I think 3 does not make sense and would submit a patch for 1 or 2 if
>>>>> someone (Stefano or you?) tells me which.
>>>> 
>>>> I have a slight preference for solution 1), and we're already using
>>>> "N/A" at least in another case (for PTS == AV_NOPTS_VALUE), so if
>>>> there are not other opinions that should be fine.
>>>> 
>>> 
>>> 
>>> makes sense. I would submit a patch. However, I saw that you already
>>> factored out functions for the two cases where you already use "N/A"
>>> and I'm not quite sure how to implement it in a way that is most
>>> consistent. One function for each case seems overkill
>>> (i.e. "nb_frames_string" analogous to "ts_value_string" and
>>> "time_value_string") and a generic "int64_value_string" outputting
>>> "N/A" for values > 0 feels odd. Any preferences? The only thing I
>>> think is a no-brainer is to define "N/A" as a constant.
>> 
>> Yes the simplest solution looks the best one in this case.
>> 
>> Today I tried to figure out which is better between N/A and no
>> printing at all, both solutions have their pros and cons, but at the
>> end I slightly prefer the "N/A" solution as it conveys more
>> information to the human reader, same for the other nb_frames pending
>> patch.
> 
> After more thought:
> 1) 0:N with N != 0 is unknown
> 2) N:0 is undefined / Non/Acceptable = N/A
> 
> For the case 1) we may either don't print at all either print
> "unknown" like it is done in other cases, so I tend to prefer this for
> consistency, but I'd like to hear other opinions before to apply that
> solution.

Hmm, I interpreted "N/A" as not available (http://en.wikipedia.org/wiki/N/A), i.e. unknown, and I do not think that another state is required or meaningful/intuitive. IMHO in all cases I have seen in ffprobe a value is either not available (unknown) for whatever reason (not implemented, or not there by definition, e.g. because the codec/container format doesn't provide it) or it is there. It makes sense to treat the unknown case consistently (not print the key value pair, print "N/A" or print "unknown") but IMHO not to differentiate between subcases of unknown.

My 2c.

Cheers,

Robert