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

[Michael Niedermayer]

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


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120920/caf1c6be/attachment.asc>