[FFmpeg-devel] [PATCH v1 2/2] fftools/ffmpeg: add exif orientation support per frame's metadata

[Nicolas George]

Jun Li (12019-05-24):
> Fix #6945
> Rotate or/and flip frame according to frame's metadata orientation

Since ffmpeg does not handle resolution changes very well, do we want
this enabled by default?

> ---
>  fftools/ffmpeg.c        |  3 ++-
>  fftools/ffmpeg.h        |  3 ++-
>  fftools/ffmpeg_filter.c | 19 ++++++++++++++++++-
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..da4c19c782 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2142,7 +2142,8 @@ static int ifilter_send_frame(InputFilter *ifilter, AVFrame *frame)
>          break;
>      case AVMEDIA_TYPE_VIDEO:
>          need_reinit |= ifilter->width  != frame->width ||
> -                       ifilter->height != frame->height;
> +                       ifilter->height != frame->height ||

> +                       ifilter->orientation != get_frame_orientation(frame);

I the filter chain does not really need reinit if the orientation change
does not change the output resolution.

It can happen for photos who were all taken in portrait mode, but
sometimes by leaning on the left and sometimes leaning on the right. I
am sorry if these questions I raise seem to complicate matters, but it
is not on purpose: automatic transformation is an issue that has many
direct consequences on the usability of the tools, they need to be at
least considered in the solution.

>          break;
>      }
>  
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index eb1eaf6363..54532ef0eb 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -244,7 +244,7 @@ typedef struct InputFilter {
>      // parameters configured for this input
>      int format;
>  
> -    int width, height;
> +    int width, height, orientation;
>      AVRational sample_aspect_ratio;
>  
>      int sample_rate;
> @@ -649,6 +649,7 @@ int init_complex_filtergraph(FilterGraph *fg);
>  void sub2video_update(InputStream *ist, AVSubtitle *sub);
>  
>  int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame *frame);
> +int get_frame_orientation(const AVFrame* frame);
>  
>  int ffmpeg_parse_options(int argc, char **argv);
>  
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index 72838de1e2..b230dafdc9 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -743,6 +743,18 @@ static int sub2video_prepare(InputStream *ist, InputFilter *ifilter)
>      return 0;
>  }
>  
> +int get_frame_orientation(const AVFrame *frame)
> +{
> +    AVDictionaryEntry *entry = NULL;
> +    int orientation = 1; // orientation indicates 'Normal' 
> +    
> +    // read exif orientation data
> +    entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);

> +    if (entry)
> +        orientation = atoi(entry->value);
> +    return orientation > 8 ? 1 : orientation;

I do not like the defensive programming here. If orientation is invalid,
then it should not be invented, even a same default value.

> +}
> +
>  static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>                                          AVFilterInOut *in)
>  {
> @@ -809,7 +821,11 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>      if (ist->autorotate) {
>          double theta = get_rotation(ist->st);
>  

> -        if (fabs(theta - 90) < 1.0) {
> +        if (fabs(theta) < 1.0) { // no rotation info in stream meta
> +            char transpose_args[32];
> +            snprintf(transpose_args, sizeof(transpose_args), "orientation=%i", ifilter->orientation);
> +            ret = insert_filter(&last_filter, &pad_idx, "transpose", transpose_args);

If the frame does not contain orientation metadata, I think the filter
should not be inserted.

> +        } else if (fabs(theta - 90) < 1.0) {
>              ret = insert_filter(&last_filter, &pad_idx, "transpose", "clock");
>          } else if (fabs(theta - 180) < 1.0) {
>              ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);

If transpose can take care of all cases, then the other cases need to be
removed, or am I missing something?

> @@ -1191,6 +1207,7 @@ int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame *frame)
>      ifilter->width               = frame->width;
>      ifilter->height              = frame->height;
>      ifilter->sample_aspect_ratio = frame->sample_aspect_ratio;
> +    ifilter->orientation         = get_frame_orientation(frame);
>  
>      ifilter->sample_rate         = frame->sample_rate;
>      ifilter->channels            = frame->channels;

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190525/9e54158a/attachment.sig>