On Sat, May 25, 2019 at 2:53 AM Nicolas George <geo...@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
