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

[David Conrad]

On Aug 10, 2007, at 4:23 PM, Robert Swain wrote:

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

I think I fixed this correctly now.