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

[Baptiste Coudurier]

On 1/16/11 6:01 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.
> [...]
> 
>> * 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

What are you talking about ? I remove the picref->pixel_aspect field.
The corresponding change must be to use
AVFilterLink->sample_aspect_ratio. This hunk is correct.

The initialization of the source filter use 1:1 if it's undefined,
"undefined" doesn't fit in IMHO.

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