[FFmpeg-devel] [RCF] lavfi aspect ratio setting path
Baptiste Coudurier
baptiste.coudurier
Mon Jan 17 03:37:00 CET 2011
On 1/16/11 6:18 PM, Michael Niedermayer wrote:
> On Sun, Jan 16, 2011 at 06:06:06PM -0800, Baptiste Coudurier wrote:
>> 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.
>
> i talk about this:
> Index: libavfilter/vf_transpose.c
> ===================================================================
> --- libavfilter/vf_transpose.c (revision 26397)
> +++ libavfilter/vf_transpose.c (working copy)
> @@ -102,6 +102,9 @@
> outlink->w = inlink->h;
> outlink->h = inlink->w;
>
> + outlink->sample_aspect_ratio.num = inlink->sample_aspect_ratio.den;
> + outlink->sample_aspect_ratio.den = inlink->sample_aspect_ratio.num;
> +
> av_log(ctx, AV_LOG_INFO, "w:%d h:%d dir:%d -> w:%d h:%d rotation:%s vflip:%d\n",
> inlink->w, inlink->h, trans->dir, outlink->w, outlink->h,
> trans->dir == 1 || trans->dir == 3 ? "clockwise" : "counterclockwise",
> @@ -117,13 +120,6 @@
> outlink->w, outlink->h);
> outlink->out_buf->pts = picref->pts;
>
> - if (picref->video->pixel_aspect.num == 0) {
> - outlink->out_buf->video->pixel_aspect = picref->video->pixel_aspect;
> - } else {
> - outlink->out_buf->video->pixel_aspect.num = picref->video->pixel_aspect.den;
> - outlink->out_buf->video->pixel_aspect.den = picref->video->pixel_aspect.num;
> - }
> -
> avfilter_start_frame(outlink, avfilter_ref_buffer(outlink->out_buf, ~0));
> }
>
> the code before keep 0 as 0 but afterwards turn 0 to infinity
inlink->sample_aspect_ratio must not be 0. "undefined" does not exist,
"undefined" is 1:1 IMHO.
--
Baptiste COUDURIER
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list