[FFmpeg-devel] [RCF] lavfi aspect ratio setting path

[Michael Niedermayer]

On Mon, Jan 17, 2011 at 02:52:29AM +0100, Michael Niedermayer wrote:
> On Sun, Jan 16, 2011 at 04:16:01PM -0800, Baptiste Coudurier wrote:
> > On 1/16/11 4:00 PM, Michael Niedermayer wrote:
> > > On Sun, Jan 16, 2011 at 03:23:23PM -0800, Baptiste Coudurier wrote:
> > >> On 1/16/11 3:20 PM, Michael Niedermayer wrote:
> > >>> On Sun, Jan 16, 2011 at 04:56:42PM +0100, Stefano Sabatini wrote:
> > >>>> On date Sunday 2010-12-12 12:34:10 -0800, Baptiste Coudurier encoded:
> > >>>>> On 12/3/10 9:53 PM, Baptiste Coudurier wrote:
> > >>>>>> On 12/2/10 6:28 PM, Michael Niedermayer wrote:
> > >>>>>>> On Mon, Nov 29, 2010 at 03:26:40AM -0800, Baptiste Coudurier wrote:
> > >>>>>>> [...]
> > >>>>>>>> @@ -419,6 +430,10 @@
> > >>>>>>>>  
> > >>>>>>>>      codec->width  = ist->output_video_filter->inputs[0]->w;
> > >>>>>>>>      codec->height = ist->output_video_filter->inputs[0]->h;
> > >>>>>>>> +    ost->st->sample_aspect_ratio = codec->sample_aspect_ratio =
> > >>>>>>>
> > >>>>>>>> +        frame_aspect_ratio == 0 ? // overriden by the -aspect cli option
> > >>>>>>>> +        av_d2q(frame_aspect_ratio*codec->height/codec->width, 255) :
> > >>>>>>>> +        ist->output_video_filter->inputs[0]->sample_aspect_ratio;
> > >>>>>>>
> > >>>>>>> that looks odd if frame_aspect_ratio == 0 then
> > >>>>>>>  av_d2q(frame_aspect_ratio*codec->height/codec->width, 255)
> > >>>>>>>  will be used
> > >>>>>>>  but thats av_d2q(0*codec->height/codec->width, 255)=0
> > >>>>>>
> > >>>>>> Yes you are right, it required more modifications to make it work.
> > >>>>>> Updated patch.
> > >>>>>>
> > >>>>>
> > >>>>> Ping.
> > >>>>
> > >>>> Any news on this? This is a show-stopper for many users.
> > >>>
> > >>> this patch does so many unrelated things iam just confused by it, i know
> > >>> some of the changes are definitly wrong and introduce bugs then for some
> > >>> i have no idea at all what they are supposed to do
> > >>
> > >> Like what ?
> > > 
> > > the vf_scale change
> > > after it there is no code left touching aspect in it but if you scale
> > > 320x240 to 320x320 then the sample aspect ratio changes.
> > 
> > Well, currently -s does not change it, why -vf scale should change it ?
> 
> because if the aspect is correct before scale it wont be correct anymore
> afterwards this happens for example in a real 640x480 ->320x200 rescaling
> 
> 
> >
> > > There are changes in ffmpeg to code only excuted for stream copy (aka no
> > > filters, no scaling) that cant be correct, the stream copy handling should not
> > > change.
> > 
> > You cannot set frame_aspect_ratio unconditionally anymore if you want
> > "-aspect" to work with libavfilter.
> > And -aspect is required to work for the stream copy case., which makes
> > the cli very inconsistent if "-aspect" does not work when encoding.
> > 
> > I double checked my tree and I don't have these changes, this must be an
> > old patch.
> > 
> > >> We agreed that sample_aspect_ratio must be added to AVFilterLink, then
> > >> according to this, ffmpeg.c is modified.
> > > 
> > > Theres nothing wrong with adding it to AVFilterLink.
> > > 
> > > But this whole thread IIRC started due to the command line option -aspect not
> > > working. This can be fixed by inserting a filter that sets the aspect instead
> > > of a second codepath to mess with the aspect. (we missed this option it seems
> > > or maybe the suggested solution looked simpler i dont remember...)
> > 
> > Not only, aspect ratio information is not printed correctly in
> > dump_format and this heavily confuses people.
> >
> 
> 
> > > And this patch looks like it attempts to do both at once. which i dont think
> > > is a good idea.
> > 
> > I don't care about this, I want the situation fixed. It's been two
> 
> Well as is i cannot review this patch because quite a few hunks make no sense
> and i dont even know which of the now 5+ things this patch combines they are
> related to.
[...]

> * unrelated simplification of vf_transpose that might or might not break it

breaks, as aspect is defined like this in avcodec and coped from and to it
over avfilter
    /**
     * sample aspect ratio (0 if unknown)
     * That is the width of a pixel divided by the height of the pixel.
     * Numerator and denominator must be relatively prime and smaller than 256 for some video standards.
     * - encoding: Set by user.
     * - decoding: Set by libavcodec.
     */
    AVRational sample_aspect_ratio;

the change turns 0 into inf

not saying this definition is smart, IMHO it should be 0/0 for undefined but
thats not how the current ABI/API defines it

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110117/0f3b9282/attachment.pgp>