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

[Stefano Sabatini]

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:
> >> 
> >> 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.

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.

Regards.
-- 
FFmpeg = Fancy & Fundamental Mega Prodigious Extended Game