| Message ID | 20190607032815.32465-2-junli1026@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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; > + } > +} [...]
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".
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,
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".
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;