[FFmpeg-devel] [PATCH] mxfdec: export aspect information.

[Reimar Döffinger]

On 18 Sep 2012, at 12:28, Tomas Härdin <tomas.hardin at codemill.se> wrote:
> On Mon, 2012-09-17 at 20:52 +0200, Reimar Döffinger wrote:
>> On Mon, Sep 17, 2012 at 11:02:33AM +0200, Tomas Härdin wrote:
>>> On Sat, 2012-09-15 at 22:37 +0200, Reimar Döffinger wrote:
>>>> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>>>> ---
>>>> libavformat/mxfdec.c |    3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>>>> index e55c490..804975e 100644
>>>> --- a/libavformat/mxfdec.c
>>>> +++ b/libavformat/mxfdec.c
>>>> @@ -1523,6 +1523,9 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>>>>                 default:
>>>>                     av_log(mxf->fc, AV_LOG_INFO, "Unknown frame layout type: %d\n", descriptor->frame_layout);
>>>>             }
>>>> +            if (descriptor->aspect_ratio.num > 0 && descriptor->aspect_ratio.den > 0)
>>>> +                st->sample_aspect_ratio = av_div_q(descriptor->aspect_ratio,
>>>> +                    (AVRational){st->codec->width, st->codec->height});
>>> 
>>> This is wrong. st->codec->width/height are StoredWidth/Height.
>> 
>> Uh, then codec->with and codec->height are set wrong,
>> coded_width/coded_height are for storing the encoded dimensions.
> 
> So it shouldn't set codec->width and codec->height at all, but coded_*
> instead? I'm confused..

All other formats set width/height to the display size.
For example for 1080p codec->height will be 1080, not 1088 even though that is the actual size.
Problem with using DisplayHeight is that since we do not support cropping the top it will probably result in cropping some video data at the bottom while keeping the VBI lines...

> I say it's wrong because you're not supposed to use the encoded
> dimensions to compute SAR - you're supposed to use the dimensions of the
> display rectangle, which is often smaller than the dimensions of the
> essence.

Yes, I just wasn't aware that it contains the coded size, that does not match other containers.

>>> (DisplayWidth/Height) if set. It's also possible for StoredWidth/Height
>>> to be wrong (certain P2 files), in which case you must wait for the
>>> dimensions to be probed before SAR is computed.
>> 
>> I very much don't think that broken files should be a significant
>> concern, at least not when it is at the expense of correct files.
> 
> Yes, I'm actually leaning towards this too. I don't think it's illegal
> for an MXF file to not specify these dimensions at all though, in which
> case you'd have to probe anyway.
> 
> For now a simple solution to this that should at least not mess anything
> up is:

I might look into this, if we agree on a view of what should be stored in width/height.