[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.
> * fix vf_pad,
> * break vf_scale,
> * unrelated simplification of vf_transpose that might or might not break it

> * reanme aspect_ratio to sample_aspect_ratio
> * pass aspect differently through libavfilter
> * maybe fix -aspect

not sure to what this belongs
 @@ -113,7 +113,6 @@ typedef struct AVFilterBufferRefAudioProps {
 typedef struct AVFilterBufferRefVideoProps {
     int w;                      ///< image width
     int h;                      ///< image height
-    AVRational pixel_aspect;    ///< pixel aspect ratio
     int interlaced;             ///< is frame interlaced
     int top_field_first;        ///< field order
 } AVFilterBufferRefVideoProps;

this is wrong

aspect is a property of frames like width and height. and can change per
frame in rare occasions.
Its ok to add it to link  if we need this but not ok to remove
from frames. we need it there as otherwise frame reordering will gurantee
we have some frames with wrong aspect during a switch


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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/c204ef6c/attachment.pgp>