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

[Jun Li]

On Sat, May 25, 2019 at 2:53 AM Nicolas George <george at nsup.org> wrote:

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


Thanks Nicolas for review.
I believe it has been enabled by default, the 'autorotate' value is true 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.
>

Do you mean the orientation case 1, 2, 3, 4 have the same resolution and 5,
6, 7, 8 are rotated, so the reinit is only necessary when switch between
these two cases?
I think it is doable. That transpose filter can do partially dynamic
processing based on frame's orientation, but the filter args still need to
set resolution info.

Correct me if I wrongly understand you question. Thanks.



> >          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.
>
Addressed in the new version.

> +}
> > +
> >  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.
>
Addressed in the new version.


> > +        } 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?
>
Fixed in the new version.

Thanks for the review !

Best Regards,
-Jun

> > @@ -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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".