[FFmpeg-cvslog] ffmpeg: fix aspect ratio setting

[Baptiste Coudurier]

On 04/06/2011 10:44 AM, Stefano Sabatini wrote:
> On date Tuesday 2011-04-05 15:02:08 -0700, Baptiste Coudurier wrote:
>> On 04/05/2011 12:09 PM, Baptiste Coudurier wrote:
> [...]
>>>> That said, I don't see a sane way to make -vf and -aspect convive when
>>>> filtering is enabled, that is i'd tend to say to the user to just use
>>>> -vf when she wants to change the final aspect (and keep -aspect for
>>>> -vcodec copy).
>>>
>>> Having 2 options is confusing for the user.
>>>
>>> I think the correct solution is to avoid adding filters when -aspect is
>>> specified on the commandline, and check frame_aspect_ratio directly.
>>> My first patch did this. It's easy to distinguish between -aspect
>>> specified or not.
>>>
>>> If it was, override the output aspect ratio no matter what (at the end
>>> of configure_filters)
>>>
>>
>> Patch attached.
>>
>> -- 
>> Baptiste COUDURIER
>> Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
>> FFmpeg maintainer                                  http://www.ffmpeg.org
> 
>> From 4ebbb6a5f34dd8d39610a9185eac5f7a925f2141 Mon Sep 17 00:00:00 2001
>> From: Baptiste Coudurier <baptiste.coudurier at gmail.com>
>> Date: Tue, 5 Apr 2011 13:37:11 -0700
>> Subject: [PATCH] Fix -aspect cli option
> 
> Please extend the commit log so that we have some more information
> stored in metadata for the next poor guy trying to figure this out.

Please suggest another message.

>> ---
>>  ffmpeg.c |   33 ++++++++++++++-------------------
>>  1 files changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 0ea369e..ccf1867 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -149,7 +149,6 @@ static int nb_streamid_map = 0;
>>  static int frame_width  = 0;
>>  static int frame_height = 0;
>>  static float frame_aspect_ratio = 0;
>> -static int frame_aspect_ratio_override = 0;
>>  static enum PixelFormat frame_pix_fmt = PIX_FMT_NONE;
>>  static int frame_bits_per_raw_sample = 0;
>>  static enum AVSampleFormat audio_sample_fmt = AV_SAMPLE_FMT_NONE;
>> @@ -285,6 +284,8 @@ typedef struct AVOutputStream {
>>      int resample_width;
>>      int resample_pix_fmt;
>>  
>> +    float frame_aspect_ratio;
> 
> I suggest to change this to "display_aspect_ratio" in a successive
> patch (as there is already enough confusion with display/pixel/sample
> aspect ratio).

Feel free to do it.

>> +
>>      /* full frame size of first frame */
>>      int original_height;
>>      int original_width;
>> @@ -429,6 +430,8 @@ static int configure_filters(AVInputStream *ist, AVOutputStream *ost)
>>      codec->width  = ost->output_video_filter->inputs[0]->w;
>>      codec->height = ost->output_video_filter->inputs[0]->h;
>>      codec->sample_aspect_ratio = ost->st->sample_aspect_ratio =
>> +        ost->frame_aspect_ratio ? // overriden by the -aspect cli option
>> +        av_d2q(ost->frame_aspect_ratio*codec->height/codec->width, 255) :
>>          ost->output_video_filter->inputs[0]->sample_aspect_ratio;
>>  
>>      return 0;
>> @@ -1695,7 +1698,7 @@ static int output_packet(AVInputStream *ist, int ist_index,
>>                              break;
>>                          case AVMEDIA_TYPE_VIDEO:
> 
>>  #if CONFIG_AVFILTER
>> -                            if (ost->picref->video)
>> +                            if (ost->picref->video && !ost->frame_aspect_ratio)
>>                                  ost->st->codec->sample_aspect_ratio = ost->picref->video->pixel_aspect;
>>  #endif
> 
> I wonder if this is required at all.

Did you test without ? If not do it.

>>                              do_video_out(os, ost, ist, &picture, &frame_size);
>> @@ -2242,6 +2245,13 @@ static int transcode(AVFormatContext **output_files,
>>                  codec->width = icodec->width;
>>                  codec->height = icodec->height;
>>                  codec->has_b_frames = icodec->has_b_frames;
> 
>> +                if (!codec->sample_aspect_ratio.num) {
>> +                    codec->sample_aspect_ratio =
>> +                    ost->st->sample_aspect_ratio =
>> +                        ist->st->sample_aspect_ratio.num ? ist->st->sample_aspect_ratio :
>> +                        ist->st->codec->sample_aspect_ratio.num ?
>> +                        ist->st->codec->sample_aspect_ratio : (AVRational){0, 1};
>> +                }
> 
> Nit+: maybe ist->st->codec -> icodec for brevity.

ist->st is common, I prefer it this way.

[...]

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