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

[Robert Krüger]

On 19.04.2010, at 23:39, Stefano Sabatini wrote:

> On date Friday 2010-04-16 18:40:53 +0200, Robert Kr?ger encoded:
>> 
>> On 16.04.2010, at 18:16, Michael Niedermayer wrote:
>> 
>>> On Thu, Apr 15, 2010 at 03:05:30PM +0200, Robert Kr?ger wrote:
>>>> 
>>>> On 15.04.2010, at 14:02, Michael Niedermayer wrote:
>>>> 
>>>>> On Thu, Apr 15, 2010 at 01:16:05PM +0200, Robert Kr?ger wrote:
>>>>>> 
>>>>>> the following patch fixes current behaviour where for example
>>>>>> for all ffmpeg-generated mjpeg files sample aspect ratio is
>>>>>> output as 0:1 and display aspect ratio is computed accordingly
>>>>>> (i.e. wrong).
>>>>> 
>>>>> undefined and 1:1 are 2 different things
>>>>> 
>>>> 
>>>> What currently happens is that obviously for some files with a
>>>> sample aspect ratio of 1:1 the corresponding struct is filled
>>>> correctly and for some it seems to be filled with 0:1.
>>> 
>>> If a field is filled incorrectly a bugreport is needed
>>> 
>> 
>> OK, I'll file one, then you can decide if it's really a bug and
>> close it if it's not.
>> 
>>> 
>>>> I looked at the way this is treated in avcodec_string in utils.c
>>>> and line 857 suggests that it treats a zero numerator for sample
>>>> aspect ratio as 1:1
>>> 
>>> no it does not
>>> 
>> 
>> 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.
 
Regards,

Robert