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

Submitted by Jun Li on June 7, 2019, 3:28 a.m.

Details

Message ID 20190607032815.32465-2-junli1026@gmail.com
State New
Headers show

Commit Message

Jun Li June 7, 2019, 3:28 a.m.
Fix #6945
Rotate or/and flip frame according to frame's metadata orientation
---
 fftools/ffmpeg.c        |  5 +++--
 fftools/ffmpeg.h        |  8 ++++++++
 fftools/ffmpeg_filter.c | 36 +++++++++++++++++++++++++++++++-----
 3 files changed, 42 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer June 7, 2019, 6:54 p.m.
On Thu, Jun 06, 2019 at 08:28:15PM -0700, Jun Li wrote:
> Fix #6945
> Rotate or/and flip frame according to frame's metadata orientation
> ---
>  fftools/ffmpeg.c        |  5 +++--
>  fftools/ffmpeg.h        |  8 ++++++++
>  fftools/ffmpeg_filter.c | 36 +++++++++++++++++++++++++++++++-----
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..bc0cece59d 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2141,8 +2141,9 @@ static int ifilter_send_frame(InputFilter *ifilter, AVFrame *frame)
>                         ifilter->channel_layout != frame->channel_layout;
>          break;
>      case AVMEDIA_TYPE_VIDEO:
> -        need_reinit |= ifilter->width  != frame->width ||
> -                       ifilter->height != frame->height;
> +        need_reinit |= ifilter->width       != frame->width ||
> +                       ifilter->height      != frame->height ||
> +                       ifilter->orientation != get_frame_orientation(frame);
>          break;
>      }
>  
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 7b6f802082..7324813ce3 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -232,6 +232,12 @@ typedef struct OptionsContext {
>      int        nb_enc_time_bases;
>  } OptionsContext;
>  
> +enum OrientationType {
> +    ORIENTATION_NONE,
> +    ORIENTATION_AUTO_FLIP,
> +    ORIENTATION_AUTO_TRANSPOSE
> +};
> +
>  typedef struct InputFilter {
>      AVFilterContext    *filter;
>      struct InputStream *ist;
> @@ -245,6 +251,7 @@ typedef struct InputFilter {
>      int format;
>  
>      int width, height;
> +    enum OrientationType orientation;
>      AVRational sample_aspect_ratio;
>  
>      int sample_rate;
> @@ -649,6 +656,7 @@ int init_complex_filtergraph(FilterGraph *fg);
>  void sub2video_update(InputStream *ist, AVSubtitle *sub);
>  
>  int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame *frame);
> +enum OrientationType 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..790751f47f 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -743,6 +743,28 @@ static int sub2video_prepare(InputStream *ist, InputFilter *ifilter)
>      return 0;
>  }
>  
> +enum OrientationType get_frame_orientation(const AVFrame *frame)
> +{
> +    AVDictionaryEntry *entry = NULL;
> +    int orientation = 0;
> +    
> +    // read exif orientation data
> +    entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);
> +    if (entry && entry->value)
> +        orientation = atoi(entry->value);

what if entry->value equals "The cow says moo" ?
This is of course a silly example but the following code would not detect it as an error
as atoi would return 0 
or is there something that prevents this case ?


> +
> +    if (orientation > 8 || orientation < 0) {
> +        av_log(NULL, AV_LOG_WARNING, "Invalid frame orientation: %i, skip it.\n", orientation);
> +        return ORIENTATION_NONE;
> +    } else if (orientation <= 1) {
> +        return ORIENTATION_NONE;
> +    } else if (orientation <= 4) {
> +        return ORIENTATION_AUTO_FLIP;
> +    } else {
> +        return ORIENTATION_AUTO_TRANSPOSE;
> +    }
> +}

[...]
Jun Li June 7, 2019, 9:45 p.m.
On Fri, Jun 7, 2019 at 11:54 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Thu, Jun 06, 2019 at 08:28:15PM -0700, Jun Li wrote:
> > Fix #6945
> > Rotate or/and flip frame according to frame's metadata orientation
> > ---
> >  fftools/ffmpeg.c        |  5 +++--
> >  fftools/ffmpeg.h        |  8 ++++++++
> >  fftools/ffmpeg_filter.c | 36 +++++++++++++++++++++++++++++++-----
> >  3 files changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 01f04103cf..bc0cece59d 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2141,8 +2141,9 @@ static int ifilter_send_frame(InputFilter
> *ifilter, AVFrame *frame)
> >                         ifilter->channel_layout != frame->channel_layout;
> >          break;
> >      case AVMEDIA_TYPE_VIDEO:
> > -        need_reinit |= ifilter->width  != frame->width ||
> > -                       ifilter->height != frame->height;
> > +        need_reinit |= ifilter->width       != frame->width ||
> > +                       ifilter->height      != frame->height ||
> > +                       ifilter->orientation !=
> get_frame_orientation(frame);
> >          break;
> >      }
> >
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index 7b6f802082..7324813ce3 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -232,6 +232,12 @@ typedef struct OptionsContext {
> >      int        nb_enc_time_bases;
> >  } OptionsContext;
> >
> > +enum OrientationType {
> > +    ORIENTATION_NONE,
> > +    ORIENTATION_AUTO_FLIP,
> > +    ORIENTATION_AUTO_TRANSPOSE
> > +};
> > +
> >  typedef struct InputFilter {
> >      AVFilterContext    *filter;
> >      struct InputStream *ist;
> > @@ -245,6 +251,7 @@ typedef struct InputFilter {
> >      int format;
> >
> >      int width, height;
> > +    enum OrientationType orientation;
> >      AVRational sample_aspect_ratio;
> >
> >      int sample_rate;
> > @@ -649,6 +656,7 @@ int init_complex_filtergraph(FilterGraph *fg);
> >  void sub2video_update(InputStream *ist, AVSubtitle *sub);
> >
> >  int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame
> *frame);
> > +enum OrientationType 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..790751f47f 100644
> > --- a/fftools/ffmpeg_filter.c
> > +++ b/fftools/ffmpeg_filter.c
> > @@ -743,6 +743,28 @@ static int sub2video_prepare(InputStream *ist,
> InputFilter *ifilter)
> >      return 0;
> >  }
> >
> > +enum OrientationType get_frame_orientation(const AVFrame *frame)
> > +{
> > +    AVDictionaryEntry *entry = NULL;
> > +    int orientation = 0;
> > +
> > +    // read exif orientation data
> > +    entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);
> > +    if (entry && entry->value)
> > +        orientation = atoi(entry->value);
>
> what if entry->value equals "The cow says moo" ?
> This is of course a silly example but the following code would not detect
> it as an error
> as atoi would return 0
> or is there something that prevents this case ?
>

Hi Michael,
The question is like how to validate a integer number, or maybe partially
validate a number.
In this case, I can do a simple check before call atoi,  but need to cover
the following case:
  "      +2"   // start with space,  with or without sign

Something might be like:
  int i = 0;
  char *pos = NULL;
  if (entry && entry->value) {
      while(entry->value[i] && entry->value[i] == ' ') // eat white-space
           i++;
      if (entry->value[i]) {
          if (entry->value[i] == '+')   // eat '+'
             i++;

          if (entry->value[i] && entry->value[i] >= '0' && entry->value <=
'8')
              pos = entry->value + i;
      }
  }

  if (pos) {
     orientation = atoi(pos);
  }

But the validation is *NOT COMPLETE* although it covers the case your
mentioned.
Like "3.4" is valid and converted to orientation 3, even "3     4" is a
valid orientation.
I think a complete validation should be creating a function like "atodigit"
, string to digit, instead of using atoi,

If you prefer a complete solution, I can send a new version to validate the
value.
What do you think ?

Best Regards,
Jun


> > +
> > +    if (orientation > 8 || orientation < 0) {
> > +        av_log(NULL, AV_LOG_WARNING, "Invalid frame orientation: %i,
> skip it.\n", orientation);
> > +        return ORIENTATION_NONE;
> > +    } else if (orientation <= 1) {
> > +        return ORIENTATION_NONE;
> > +    } else if (orientation <= 4) {
> > +        return ORIENTATION_AUTO_FLIP;
> > +    } else {
> > +        return ORIENTATION_AUTO_TRANSPOSE;
> > +    }
> > +}
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> "Nothing to hide" only works if the folks in power share the values of
> you and everyone you know entirely and always will -- Tom Scott
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George June 8, 2019, 9:24 a.m.
Jun Li (12019-06-07):
> I think a complete validation should be creating a function like "atodigit"
> , string to digit, instead of using atoi,

The problem of validation is a common one, and as such it already has a
solution.

APPLICATION USAGE

    The atoi() function is subsumed by strtol() but is retained because it is
    used extensively in existing code. If the number is not known to be in
    range, strtol() should be used because atoi() is not required to perform
    any error checking.

Regards,
Jun Li June 9, 2019, 4:39 a.m.
On Sat, Jun 8, 2019 at 2:25 AM Nicolas George <george@nsup.org> wrote:

> Jun Li (12019-06-07):
> > I think a complete validation should be creating a function like
> "atodigit"
> > , string to digit, instead of using atoi,
>
> The problem of validation is a common one, and as such it already has a
> solution.
>
> APPLICATION USAGE
>
>     The atoi() function is subsumed by strtol() but is retained because it
> is
>     used extensively in existing code. If the number is not known to be in
>     range, strtol() should be used because atoi() is not required to
> perform
>     any error checking.


Thanks Nicolas and Michael, it is very helpful.
After reread the exif orientation doc, I realized that the correct value
should be in range [1,8], that is, 0 is considered as invalid.

So the new version is still using "atoi", since it return 0 for either
input  "0" or "This is a test", and 0 considered as invalid in any case.
https://patchwork.ffmpeg.org/patch/13471/
I am not against using strtol, which is a superset of atoi. Let me know if
there is a code preference in ffmpeg since I see strtol is more widely used.

Best Regards,
Jun


>
>
Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 01f04103cf..bc0cece59d 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2141,8 +2141,9 @@  static int ifilter_send_frame(InputFilter *ifilter, AVFrame *frame)
                        ifilter->channel_layout != frame->channel_layout;
         break;
     case AVMEDIA_TYPE_VIDEO:
-        need_reinit |= ifilter->width  != frame->width ||
-                       ifilter->height != frame->height;
+        need_reinit |= ifilter->width       != frame->width ||
+                       ifilter->height      != frame->height ||
+                       ifilter->orientation != get_frame_orientation(frame);
         break;
     }
 
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index 7b6f802082..7324813ce3 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -232,6 +232,12 @@  typedef struct OptionsContext {
     int        nb_enc_time_bases;
 } OptionsContext;
 
+enum OrientationType {
+    ORIENTATION_NONE,
+    ORIENTATION_AUTO_FLIP,
+    ORIENTATION_AUTO_TRANSPOSE
+};
+
 typedef struct InputFilter {
     AVFilterContext    *filter;
     struct InputStream *ist;
@@ -245,6 +251,7 @@  typedef struct InputFilter {
     int format;
 
     int width, height;
+    enum OrientationType orientation;
     AVRational sample_aspect_ratio;
 
     int sample_rate;
@@ -649,6 +656,7 @@  int init_complex_filtergraph(FilterGraph *fg);
 void sub2video_update(InputStream *ist, AVSubtitle *sub);
 
 int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame *frame);
+enum OrientationType 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..790751f47f 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -743,6 +743,28 @@  static int sub2video_prepare(InputStream *ist, InputFilter *ifilter)
     return 0;
 }
 
+enum OrientationType get_frame_orientation(const AVFrame *frame)
+{
+    AVDictionaryEntry *entry = NULL;
+    int orientation = 0;
+    
+    // read exif orientation data
+    entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);
+    if (entry && entry->value)
+        orientation = atoi(entry->value);
+
+    if (orientation > 8 || orientation < 0) {
+        av_log(NULL, AV_LOG_WARNING, "Invalid frame orientation: %i, skip it.\n", orientation);
+        return ORIENTATION_NONE;
+    } else if (orientation <= 1) {
+        return ORIENTATION_NONE;
+    } else if (orientation <= 4) {
+        return ORIENTATION_AUTO_FLIP;
+    } else {
+        return ORIENTATION_AUTO_TRANSPOSE;
+    }
+}
+
 static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
                                         AVFilterInOut *in)
 {
@@ -809,13 +831,16 @@  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
+            if (ifilter->orientation == ORIENTATION_AUTO_FLIP) { 
+                ret = insert_filter(&last_filter, &pad_idx, "transpose", "orientation=auto_flip");
+            } else if (ifilter->orientation == ORIENTATION_AUTO_TRANSPOSE) {
+                ret = insert_filter(&last_filter, &pad_idx, "transpose", "orientation=auto_transpose");
+            }
+        } 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 (ret < 0)
-                return ret;
-            ret = insert_filter(&last_filter, &pad_idx, "vflip", NULL);
+            ret = insert_filter(&last_filter, &pad_idx, "transpose", "orientation=rotate180");
         } else if (fabs(theta - 270) < 1.0) {
             ret = insert_filter(&last_filter, &pad_idx, "transpose", "cclock");
         } else if (fabs(theta) > 1.0) {
@@ -1191,6 +1216,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;