| Message ID | 20190525000427.25546-2-junli1026@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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,
On Fri, May 24, 2019 at 05:04:27PM -0700, Jun Li wrote: > Fix #6945 > Rotate or/and flip frame according to frame's metadata orientation > --- > fftools/ffmpeg.c | 3 ++- > fftools/ffmpeg.h | 3 ++- > fftools/ffmpeg_filter.c | 19 ++++++++++++++++++- > 3 files changed, 22 insertions(+), 3 deletions(-) This breaks 227 fate tests here [...]
On Sat, May 25, 2019 at 6:16 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, May 24, 2019 at 05:04:27PM -0700, Jun Li wrote: > > Fix #6945 > > Rotate or/and flip frame according to frame's metadata orientation > > --- > > fftools/ffmpeg.c | 3 ++- > > fftools/ffmpeg.h | 3 ++- > > fftools/ffmpeg_filter.c | 19 ++++++++++++++++++- > > 3 files changed, 22 insertions(+), 3 deletions(-) > > This breaks 227 fate tests here > > My bad, forgot to run test. Sorry about that. New patch is sent out, should be fine now. Thanks. Best Regards -Jun > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Take away the freedom of one citizen and you will be jailed, take away > the freedom of all citizens and you will be congratulated by your peers > in Parliament. > _______________________________________________ > 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".
On Sat, May 25, 2019 at 2:53 AM Nicolas George <george@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-request@ffmpeg.org with subject "unsubscribe".
Jun Li (12019-05-25): > I believe it has been enabled by default, the 'autorotate' value is true by > default. Yes, but it is currently enabled for constant rotation. Variable rotation is more tricky, so the question of enabling it by default must be asked again. > 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 do not know the numeric values by rote, but I think that is what I mean. > 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. You can have the filter examine the metadata directly, as an option. > Addressed in the new version. Did you send it? If so I missed it. Regards,
On Mon, May 27, 2019 at 7:56 AM Nicolas George <george@nsup.org> wrote: > Jun Li (12019-05-25): > > I believe it has been enabled by default, the 'autorotate' value is true > by > > default. > > Yes, but it is currently enabled for constant rotation. Variable > rotation is more tricky, so the question of enabling it by default must > be asked again. > > > 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 do not know the numeric values by rote, but I think that is what I > mean. > > > 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. > > You can have the filter examine the metadata directly, as an option. > > > Addressed in the new version. > > Did you send it? If so I missed it. > > Hi Nicolas, I updated the version as follows, it addresses your feedback and leveraged current flip functionality, as well new option to auto-flip or auto-transpose. https://patchwork.ffmpeg.org/patch/13311/ https://patchwork.ffmpeg.org/patch/13312/ Thanks for your review. 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..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); 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; +} + 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); + } 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); @@ -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;