[FFmpeg-soc] [soc]: r656 - matroska/matroskaenc.c

[Robert Swain]

On 10/08/07, Aurelien Jacobs <aurel at gnuage.org> wrote:
> On Fri, 10 Aug 2007 11:08:04 +0100
> "Robert Swain" <robert.swain at gmail.com> wrote:
>
> > On 10/08/07, Aurelien Jacobs <aurel at gnuage.org> wrote:
> > > On Fri, 10 Aug 2007 01:37:12 +0200 (CEST)
> > > conrad <subversion at mplayerhq.hu> wrote:
> > >
> > > > Author: conrad
> > > > Date: Fri Aug 10 01:37:12 2007
> > > > New Revision: 656
> > > >
> > > > Log:
> > > > Write the display size elements
> > > >
> > > >
> > > > Modified:
> > > >    matroska/matroskaenc.c
> > > >
> > > > Modified: matroska/matroskaenc.c
> > > > ==============================================================================
> > > > --- matroska/matroskaenc.c    (original)
> > > > +++ matroska/matroskaenc.c    Fri Aug 10 01:37:12 2007
> > > > @@ -476,7 +476,10 @@ static int mkv_write_tracks(AVFormatCont
> > > >                  // XXX: interlace flag?
> > > >                  put_ebml_uint (pb, MATROSKA_ID_VIDEOPIXELWIDTH , codec->width);
> > > >                  put_ebml_uint (pb, MATROSKA_ID_VIDEOPIXELHEIGHT, codec->height);
> > > > -                // XXX: display width/height
> > > > +                if (codec->sample_aspect_ratio.num) {
> > > > +                    put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , codec->sample_aspect_ratio.num);
> > > > +                    put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, codec->sample_aspect_ratio.den);
> > >
> > > I'm not sure, but I suppose that sample_aspect_ratio is something
> > > different than videodisplaywidth/height.
> > > I guess that you could have sample_aspect_ratio = 4/3 when
> > > videodisplaywidth/height = 640/480.
> > > Anyone can confirm?
> >
> > Sample aspect ratio is also known as pixel aspect ratio.
> >
> > DAR = display width / display height
> > SAR = sample width / sample height
> >
> > DAR = SAR * encoded frame width / encoded frame height
> >
> > Computer screens generally have SAR = 1 (i.e. square pixels). As I
> > recall, TV screens do not. DVDs have SARs calculated from the encoded
> > frame width and height to obtain the correct display aspect ratio
> > (either 4/3 or 16/9).
> >
> > Anyway, these values should take whatever values FFmpeg produces in my
> > opinion, so that commit looks correct.
>
> I agree that SAR values produced by ffmpeg should be right. But the
> values stored in the mkv file are in fact the DAR (well, the display
> width and height).
> So storing the SAR here still seems plain wrong to me !

Oops, I overlooked the MATROSKA_ID_VIDEODISPLAYWIDTH etc. :) You're
correct, the commit is wrong, sorry for the nouse. The DAR should be
calculated as mentioned and used instead.

Rob