[FFmpeg-cvslog] ffmpeg: fix aspect ratio setting

[Stefano Sabatini]

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.

> ---
>  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).

> +
>      /* 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.

>                              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.

>                  break;
>              case AVMEDIA_TYPE_SUBTITLE:
>                  codec->width = icodec->width;
> @@ -2903,7 +2913,6 @@ static void opt_frame_aspect_ratio(const char *arg)
>          ffmpeg_exit(1);
>      }
>      frame_aspect_ratio = ar;
> -    frame_aspect_ratio_override = 1;
>  }
>  
>  static int opt_metadata(const char *opt, const char *arg)
> @@ -3339,12 +3348,6 @@ static void opt_input_file(const char *filename)
>              set_context_opts(dec, avcodec_opts[AVMEDIA_TYPE_VIDEO], AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM, input_codecs[nb_input_codecs-1]);
>              frame_height = dec->height;
>              frame_width  = dec->width;
> -            if(ic->streams[i]->sample_aspect_ratio.num)
> -                frame_aspect_ratio=av_q2d(ic->streams[i]->sample_aspect_ratio);
> -            else
> -                frame_aspect_ratio=av_q2d(dec->sample_aspect_ratio);
> -            frame_aspect_ratio *= (float) dec->width / dec->height;
> -            frame_aspect_ratio_override = 0;
>              frame_pix_fmt = dec->pix_fmt;
>              rfps      = ic->streams[i]->r_frame_rate.num;
>              rfps_base = ic->streams[i]->r_frame_rate.den;
> @@ -3449,7 +3452,6 @@ static void new_video_stream(AVFormatContext *oc, int file_idx)
>      AVCodecContext *video_enc;
>      enum CodecID codec_id = CODEC_ID_NONE;
>      AVCodec *codec= NULL;
> -    int i;
>  
>      st = av_new_stream(oc, oc->nb_streams < nb_streamid_map ? streamid_map[oc->nb_streams] : 0);
>      if (!st) {
> @@ -3469,15 +3471,9 @@ static void new_video_stream(AVFormatContext *oc, int file_idx)
>              codec_id = av_guess_codec(oc->oformat, NULL, oc->filename, NULL, AVMEDIA_TYPE_VIDEO);
>              codec = avcodec_find_encoder(codec_id);
>          }
> +        ost->frame_aspect_ratio = frame_aspect_ratio;
> +        frame_aspect_ratio = 0;
>  #if CONFIG_AVFILTER
> -        if(frame_aspect_ratio_override){
> -            i = vfilters ? strlen(vfilters) : 0;
> -            vfilters = av_realloc(vfilters, i+100);
> -            snprintf(vfilters+i, 100, "%csetdar=%f\n", i?',':' ', frame_aspect_ratio);
> -            frame_aspect_ratio=0;
> -            frame_aspect_ratio_override=0;
> -        }
> -
>          ost->avfilter= vfilters;
>          vfilters= NULL;
>  #endif
> @@ -3524,7 +3520,6 @@ static void new_video_stream(AVFormatContext *oc, int file_idx)
>  
>          video_enc->width = frame_width;
>          video_enc->height = frame_height;
> -        video_enc->sample_aspect_ratio = av_d2q(frame_aspect_ratio*video_enc->height/video_enc->width, 255);
>          video_enc->pix_fmt = frame_pix_fmt;
>          video_enc->bits_per_raw_sample = frame_bits_per_raw_sample;
>          st->sample_aspect_ratio = video_enc->sample_aspect_ratio;

Apart from the pointed nits looks fine to me, but make sure to test
with the complete FATE (and with -vcodec copy) so that we can avoid
more regressions.
-- 
"There are things that are so serious that you can only joke about them"
		-- Heisenberg