[FFmpeg-devel] [PATCH] mxfdec: export aspect information.
Michael Niedermayer
michaelni at gmx.at
Fri Sep 21 03:33:14 CEST 2012
On Thu, Sep 20, 2012 at 09:24:10AM +0200, Tomas Härdin wrote:
> 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
sscanf(str. "%d/%d", &r.num, &r.den);
The advantage of metadata is that it works with every format that
allows generic metadata.
If you implement it the other way around its more work for the
(de)muxers that have to store it by using metadata and less work for
the ones that store it in a way different from metadata.
> also the case of remuxing between MXF and MOV, and probably other
> formats that support this information.
you can implement it anyway you like. I just would like it to be
working with nut, mkv, mxf and mov.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20120921/ece202ff/attachment.asc>
More information about the ffmpeg-devel
mailing list