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

[Baptiste Coudurier]

On 1/16/11 6:09 PM, Michael Niedermayer wrote:
> 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

Humm well, too messy for me. I'm passing on this one.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org