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

[Tomas Härdin]

On Thu, 2012-09-20 at 05:49 +0200, Michael Niedermayer wrote:
> On Tue, Sep 18, 2012 at 12:27:40PM +0200, Tomas Härdin wrote:
> > 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.
> 
> yes, that was what i had in mind when i wrote this, iam not sure its
> a good idea though

Isn't this the very reason avformat_find_stream_info() exists in the
first place - to compute stuff the containers can't on their own? And to
do computation common to multiple demuxers?

> > > 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).
> 
> if you want the information to be copied in other formats then its
> probably best to use metadata as this doesnt need per muxer/demuxer
> code to handle the specific fields

The metadata system is a bad fit for this because parsing strings is a
pain, especially when they have to contain rational numbers. There is
also the case of remuxing between MXF and MOV, and probably other
formats that support this information.

/Tomas