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

[Tomas Härdin]

On Mon, 2012-09-17 at 15:53 +0200, Michael Niedermayer 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. SAR
> > should be computed from the size of the display rectangle
> > (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.
> > 
> > There are also files where the display resolution may be wrong - certain
> > files that pop out of Avid Media Composer have DisplayWidth =
> > SampledWidth * SAR. This can be solved by simply letting DisplayWidth =
> > SampledWidth.
> > DisplayWidth must be less than or equal to SampledWidth, same for
> > height. SampledWidth may be larger than StoredWidth IIRC, in which case
> > the frame should be padded with black. I haven't run into any such files
> > though, meaning I just pretend Sampled* don't exist (technically wrong,
> > but it works). See S377m Annex E.
> > 
> > Finally, there are files where neither rectangle is set.
> > 
> > At work I solved this by exposing DAR, DisplayWidth and DisplayHeight in
> > AVStream, then computing SAR in avformat_find_stream_info() after codec
> > dimensions have been probed. Like so:
> > 
> >      if (!st->r_frame_rate.num){
> >          if(    st->codec->time_base.den * (int64_t)st->time_base.num
> >              <= st->codec->time_base.num * st->codec->ticks_per_frame * (int64_t)st->time_base.den){
> >              st->r_frame_rate.num = st->codec->time_base.den;
> >              st->r_frame_rate.den = st->codec->time_base.num * st->codec->ticks_per_frame;
> >          }else{
> >              st->r_frame_rate.num = st->time_base.den;
> >              st->r_frame_rate.den = st->time_base.num;
> >          }
> >      }
> > +
> > +    /* if no display resolution set, take codec resolution */
> > +    if (!st->display_width.num)  st->display_width  = (AVRational){st->codec->width, 1};
> > +    if (!st->display_height.num) st->display_height = (AVRational){st->codec->height, 1};
> > +
> > +    /* if no container SAR, then compute it from container DAR and display resolution */
> > +    if (!st->sample_aspect_ratio.num && st->display_aspect_ratio.num) {
> > +        st->sample_aspect_ratio = av_div_q(st->display_aspect_ratio, av_div_q(st->display_width, st->display_height));
> > +    }
> >  }else if(st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
> > 
> > Oh yeah, you need to make sure IMX50 with VBI lines work with whatever
> > solution you come up with. So 720x608 with display rect = 720x576 @
> > (0,32). For DAR=4:3 SAR should pop out as 16:15, not whatever
> > 4:3/720:608 works out to.
> > 
> > That's my core dump on this issue for now. For extra fun you could make
> > this work for MOV too, and when muxing both MXF and MOV.
> 
> If the mxf demxuer simply doesnt set codec width height for cases that
> can be probed (all but rawvideo for the mxf case i suspect)

IMO all demuxers should do this, or the container dimensions should be
kept separate and copied by avformat_find_stream_info() into
AVCodecContext if it fails to probe them. This might be slower though.

> then the demuxer just needs to wait for w/h to become non zero and
> at that point it has all information codec dimensions, and its own
> and can calculate sar from these

Are you saying this should be computed in the first call to
mxf_read_packet()? That doesn't sound very pretty.

> No need to export the stuff and put mxf code in find_stream_info

FYI, MOV also has display rectangle stuff in the form of clean aperture
AKA "clap", so it's not MXF specific. Oh, and both MXF and MOV may or
may not have this information present. If not then the coded dimensions
should be used, which might have to be probed.

The nice thing if this information were exposed is that VBI lines and
similar crap could be hidden in ffplay and automagically cropped by
ffmpeg if desired (or this information could be written to the output so
other players also hide the crap).

/Tomas