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

[Baptiste Coudurier]

Hi guys,

On 11/20/10 8:01 AM, Michael Niedermayer wrote:
> On Sat, Nov 20, 2010 at 12:42:52PM +0100, Stefano Sabatini wrote:
>> On date Friday 2010-11-19 19:14:28 -0800, Baptiste Coudurier encoded:
>>> On 11/19/2010 06:55 PM, Michael Niedermayer wrote:
>>>> On Thu, Nov 18, 2010 at 04:45:48PM -0800, Baptiste Coudurier wrote:
>>>>> On 11/18/2010 04:33 PM, Michael Niedermayer wrote:
>>>>>> On Thu, Nov 18, 2010 at 04:12:29PM -0800, Baptiste Coudurier wrote:
>>>>>>> Hi Stefano,
>>>>>>>
>>>>>>> On 11/18/2010 01:27 PM, Stefano Sabatini wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> -aspect currently works by changing the PAR in the video encoder
>>>>>>>> context and in the output stream, thus readjusting the output DAR.
>>>>>>>>
>>>>>>>> If the libavfilter is enabled, in output_packet() we have:
>>>>>>>>       if (ist->picref->video)
>>>>>>>>           ost->st->codec->sample_aspect_ratio = ist->picref->video->pi
>>>>>>>>
>>>>>>>> so sample_aspect_ratio is set to the value in the filtered frame,
>>>>>>>> thus ignoring the value set with -aspect.
>>>>>>>>
>>>>>>>> Workaround for this issue is either configure with --disable-avfilter
>>>>>>>> and use -aspect, or use the aspect filter.
>>>>>>>>
>>>>>>>> I suggest to disable the -aspect option when using libavfilter, thus
>>>>>>>> forcing the users to employ the aspect filter. Alternatively we could
>>>>>>>> just drop support for -aspect in favor of libavfilter, but when that
>>>>>>>> approach made sense with the pad and crop options which were
>>>>>>>> "polluting" the ffmpeg code with much hard-to-maintain complex code,
>>>>>>>> this doesn't seem the case for the aspect option and I still want to
>>>>>>>> keep that option for users which for some reason (e.g. because of the
>>>>>>>> lavfi regressions) are using an ffmpeg compiled with
>>>>>>>> --disable-avfilter.
>>>>>>>
>>>>>>> First, thanks for looking into this.
>>>>>>>
>>>>>>> I, too, would like to keep the -aspect cli option.
>>>>>>>
>>>>>>> I have a patch in my tree that actually set the aspect at the end of
>>>>>>> configure_filters and disable the code you quoted above.
>>>>>>
>>>>>> can you post that to the list so i can take a look?
>>>>> >from your description it sounds like a good solution
>>>>>
>>>>> Here it is.
>>>>>
>>>>> --
>>>>> Baptiste COUDURIER
>>>>> Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
>>>>> FFmpeg maintainer                                  http://www.ffmpeg.org
>>>>
>>>>>  ffmpeg.c |    7 +++----
>>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>> 52f66ace0baf991b397cbe489f8df5b7623e0f33  0001-Fix-output-aspect-ratio-with-libavfilter.patch
>>>>> From 246c5cdba7defcd697597549fd110fb8b86cae62 Mon Sep 17 00:00:00 2001
>>>>> From: bcoudurier<baptiste.coudurier at gmail.com>
>>>>> Date: Sun, 24 Oct 2010 22:33:42 -0700
>>>>> Subject: [PATCH 1/5] Fix output aspect ratio with libavfilter
>>>>>
>>>>> ---
>>>>>  ffmpeg.c |    7 +++----
>>>>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/ffmpeg.c b/ffmpeg.c
>>>>> index 1681664..f06cc3e 100644
>>>>> --- a/ffmpeg.c
>>>>> +++ b/ffmpeg.c
>>>>> @@ -450,6 +450,9 @@ static int configure_filters(AVInputStream *ist, AVOutputStream *ost)
>>>>>
>>>>>      codec->width  = ist->output_video_filter->inputs[0]->w;
>>>>>      codec->height = ist->output_video_filter->inputs[0]->h;
>>>>> +    codec->sample_aspect_ratio =
>>>>> +        av_d2q(frame_aspect_ratio*codec->height/codec->width, 255);
>>>>> +    ost->st->sample_aspect_ratio = codec->sample_aspect_ratio;
>>>>
>>>> this looks like it would break the aspect filter, in the sense that its
>>>> value would be ignored. But maybe i miss something?
>>>
>>> Yes, it will be ignored.
>>> The aspect filter handling is weird anyway, dump_format and thus
>>> ffmpeg won't display the correct aspect ratio, I consider this
>>> broken so I disable his effects.
>>
>> The aspect filters have another problem, they may fail with
>> some encoder which needs to set the DAR/SAR when openining the
>> context, because they set DAR/SAR frame by frame (while this *may* be
>> a global property).
>>
>> If this is the case (I need confirmation from someone with more
>> expertise in encoders), then the only way to fix the problem is to
>> make the aspect a property of the link, and make it configurable
>> during the configuration stage. In case the aspect changes during
>> filtering, we may need to re-configure the filtergraph (as it should
>> happen when w/h change).
> 
> SAR should be at a similar level as w/h
> alot of encoders and formats wont allow changing sar (like they dont allow w/h
> changes)
> and a sar change if it occurs is very likely also to change the w/h
> and if we have a input that changes sar (and w/h) a scale filter somewhere
> could be used to get rid of these changes for outputs that cant handle it
> 

Great, here is a shoot at it.

Btw, I disabled the par alteration in scale filter, to keep the current
behaviour
of ffmpeg.
-aspect cli option works correctly.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: avfilter_par.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101124/89b7c650/attachment.txt>