On 6/9/19, Jun Li <junli1...@gmail.com> wrote: > On Sun, Jun 9, 2019 at 12:49 AM Paul B Mahol <one...@gmail.com> wrote: > >> On 6/9/19, Jun Li <junli1...@gmail.com> wrote: >> > Add exif orientation support and expose an option. >> > --- >> > libavfilter/hflip.h | 2 + >> > libavfilter/transpose.h | 14 ++++ >> > libavfilter/vf_hflip.c | 40 ++++++--- >> > libavfilter/vf_transpose.c | 168 ++++++++++++++++++++++++++++++++----- >> > 4 files changed, 192 insertions(+), 32 deletions(-) >> > >> > diff --git a/libavfilter/hflip.h b/libavfilter/hflip.h >> > index 204090dbb4..4e89bae3fc 100644 >> > --- a/libavfilter/hflip.h >> > +++ b/libavfilter/hflip.h >> > @@ -35,5 +35,7 @@ typedef struct FlipContext { >> > >> > int ff_hflip_init(FlipContext *s, int step[4], int nb_planes); >> > void ff_hflip_init_x86(FlipContext *s, int step[4], int nb_planes); >> > +int ff_hflip_config_props(FlipContext* s, AVFilterLink *inlink); >> > +int ff_hflip_filter_slice(FlipContext *s, AVFrame *in, AVFrame *out, >> > int >> > job, int nb_jobs, int vlifp); >> > >> > #endif /* AVFILTER_HFLIP_H */ >> > diff --git a/libavfilter/transpose.h b/libavfilter/transpose.h >> > index aa262b9487..5da08bddc0 100644 >> > --- a/libavfilter/transpose.h >> > +++ b/libavfilter/transpose.h >> > @@ -34,4 +34,18 @@ enum TransposeDir { >> > TRANSPOSE_VFLIP, >> > }; >> > >> > +enum OrientationType { >> > + ORIENTATION_AUTO_TRANSPOSE = -2, >> > + ORIENTATION_AUTO_FLIP = -1, >> > + ORIENTATION_NONE = 0, >> > + ORIENTATION_NORMAL, >> > + ORIENTATION_HFLIP, >> > + ORIENTATION_ROTATE180, >> > + ORIENTATION_VFLIP, >> > + ORIENTATION_HFLIP_ROTATE270CW, >> > + ORIENTATION_ROTATE90CW, >> > + ORIENTATION_HFLIP_ROTATE90CW, >> > + ORIENTATION_ROTATE270CW >> > +}; >> > + >> > #endif >> > diff --git a/libavfilter/vf_hflip.c b/libavfilter/vf_hflip.c >> > index b77afc77fc..d24ca5c2e7 100644 >> > --- a/libavfilter/vf_hflip.c >> > +++ b/libavfilter/vf_hflip.c >> > @@ -125,9 +125,8 @@ static void hflip_qword_c(const uint8_t *ssrc, >> uint8_t >> > *ddst, int w) >> > dst[j] = src[-j]; >> > } >> > >> > -static int config_props(AVFilterLink *inlink) >> > +int ff_hflip_config_props(FlipContext* s, AVFilterLink *inlink) >> > { >> > - FlipContext *s = inlink->dst->priv; >> > const AVPixFmtDescriptor *pix_desc = >> > av_pix_fmt_desc_get(inlink->format); >> > const int hsub = pix_desc->log2_chroma_w; >> > const int vsub = pix_desc->log2_chroma_h; >> > @@ -144,6 +143,12 @@ static int config_props(AVFilterLink *inlink) >> > return ff_hflip_init(s, s->max_step, nb_planes); >> > } >> > >> > +static int config_props(AVFilterLink *inlink) >> > +{ >> > + FlipContext *s = inlink->dst->priv; >> > + return ff_hflip_config_props(s, inlink); >> > +} >> > + >> > int ff_hflip_init(FlipContext *s, int step[4], int nb_planes) >> > { >> > int i; >> > @@ -170,14 +175,10 @@ typedef struct ThreadData { >> > AVFrame *in, *out; >> > } ThreadData; >> > >> > -static int filter_slice(AVFilterContext *ctx, void *arg, int job, int >> > nb_jobs) >> > +int ff_hflip_filter_slice(FlipContext *s, AVFrame *in, AVFrame *out, >> > int >> > job, int nb_jobs, int vflip) >> > { >> > - FlipContext *s = ctx->priv; >> > - ThreadData *td = arg; >> > - AVFrame *in = td->in; >> > - AVFrame *out = td->out; >> > uint8_t *inrow, *outrow; >> > - int i, plane, step; >> > + int i, plane, step, outlinesize; >> > >> > for (plane = 0; plane < 4 && in->data[plane] && >> > in->linesize[plane]; >> > plane++) { >> > const int width = s->planewidth[plane]; >> > @@ -187,19 +188,36 @@ static int filter_slice(AVFilterContext *ctx, void >> > *arg, int job, int nb_jobs) >> > >> > step = s->max_step[plane]; >> > >> > - outrow = out->data[plane] + start * out->linesize[plane]; >> > - inrow = in ->data[plane] + start * in->linesize[plane] + >> (width - >> > 1) * step; >> > + if (vflip) { >> > + outrow = out->data[plane] + (height - start - 1)* >> > out->linesize[plane]; >> > + outlinesize = -out->linesize[plane]; >> > + } else { >> > + outrow = out->data[plane] + start * out->linesize[plane]; >> > + outlinesize = out->linesize[plane]; >> > + } >> > + >> > + inrow = in->data[plane] + start * in->linesize[plane] + (width >> - >> > 1) * step; >> > + >> > for (i = start; i < end; i++) { >> > s->flip_line[plane](inrow, outrow, width); >> > >> > inrow += in ->linesize[plane]; >> > - outrow += out->linesize[plane]; >> > + outrow += outlinesize; >> > } >> > } >> > >> > return 0; >> > } >> > >> > +static int filter_slice(AVFilterContext *ctx, void *arg, int job, int >> > nb_jobs) >> > +{ >> > + FlipContext *s = ctx->priv; >> > + ThreadData *td = arg; >> > + AVFrame *in = td->in; >> > + AVFrame *out = td->out; >> > + return ff_hflip_filter_slice(s, in, out, job, nb_jobs, 0); >> > +} >> > + >> > static int filter_frame(AVFilterLink *inlink, AVFrame *in) >> > { >> > AVFilterContext *ctx = inlink->dst; >> > diff --git a/libavfilter/vf_transpose.c b/libavfilter/vf_transpose.c >> > index dd54947bd9..7a122ba8a9 100644 >> > --- a/libavfilter/vf_transpose.c >> > +++ b/libavfilter/vf_transpose.c >> > @@ -39,6 +39,7 @@ >> > #include "internal.h" >> > #include "video.h" >> > #include "transpose.h" >> > +#include "hflip.h" >> > >> > typedef struct TransVtable { >> > void (*transpose_8x8)(uint8_t *src, ptrdiff_t src_linesize, >> > @@ -48,16 +49,22 @@ typedef struct TransVtable { >> > int w, int h); >> > } TransVtable; >> > >> > -typedef struct TransContext { >> > - const AVClass *class; >> > +typedef struct TransContextData { >> > int hsub, vsub; >> > int planes; >> > int pixsteps[4]; >> > + TransVtable vtables[4]; >> > +} TransContextData; >> > >> > +typedef struct TransContext { >> > + const AVClass *class; >> > int passthrough; ///< PassthroughType, landscape passthrough >> > mode >> > enabled >> > int dir; ///< TransposeDir >> > - >> > - TransVtable vtables[4]; >> > + int orientation; ///< OrientationType >> > + union { >> > + TransContextData transpose; >> > + FlipContext flip; >> > + } ctx; >> > } TransContext; >> > >> > static int query_formats(AVFilterContext *ctx) >> > @@ -207,14 +214,18 @@ static int config_props_output(AVFilterLink >> *outlink) >> > s->passthrough = TRANSPOSE_PT_TYPE_NONE; >> > } >> > >> > - s->hsub = desc_in->log2_chroma_w; >> > - s->vsub = desc_in->log2_chroma_h; >> > - s->planes = av_pix_fmt_count_planes(outlink->format); >> > + if (s->orientation == ORIENTATION_AUTO_FLIP || >> > + (s->orientation > ORIENTATION_NONE && s->orientation <= >> > ORIENTATION_VFLIP)) >> > + return ff_hflip_config_props(&(s->ctx.flip), inlink); >> > + >> > + s->ctx.transpose.hsub = desc_in->log2_chroma_w; >> > + s->ctx.transpose.vsub = desc_in->log2_chroma_h; >> > + s->ctx.transpose.planes = av_pix_fmt_count_planes(outlink->format); >> > >> > av_assert0(desc_in->nb_components == desc_out->nb_components); >> > >> > >> > - av_image_fill_max_pixsteps(s->pixsteps, NULL, desc_out); >> > + av_image_fill_max_pixsteps(s->ctx.transpose.pixsteps, NULL, >> desc_out); >> > >> > outlink->w = inlink->h; >> > outlink->h = inlink->w; >> > @@ -226,10 +237,10 @@ static int config_props_output(AVFilterLink >> *outlink) >> > outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; >> > >> > for (int i = 0; i < 4; i++) { >> > - TransVtable *v = &s->vtables[i]; >> > - switch (s->pixsteps[i]) { >> > + TransVtable *v = &s->ctx.transpose.vtables[i]; >> > + switch (s->ctx.transpose.pixsteps[i]) { >> > case 1: v->transpose_block = transpose_block_8_c; >> > - v->transpose_8x8 = transpose_8x8_8_c; break; >> > + v->transpose_8x8 = transpose_8x8_8_c; break; >> > case 2: v->transpose_block = transpose_block_16_c; >> > v->transpose_8x8 = transpose_8x8_16_c; break; >> > case 3: v->transpose_block = transpose_block_24_c; >> > @@ -262,21 +273,69 @@ static AVFrame *get_video_buffer(AVFilterLink >> *inlink, >> > int w, int h) >> > >> > typedef struct ThreadData { >> > AVFrame *in, *out; >> > + int orientation; >> > } ThreadData; >> > >> > -static int filter_slice(AVFilterContext *ctx, void *arg, int jobnr, >> > - int nb_jobs) >> > +static int get_frame_orientation(AVFilterContext *ctx, const AVFrame >> > *frame) >> > +{ >> > + TransContext *s = ctx->priv; >> > + AVDictionaryEntry *entry = NULL; >> > + int orientation = ORIENTATION_NONE; >> > + >> > + if (s->orientation >= ORIENTATION_NONE) >> > + return s->orientation; >> > + >> > + // read exif orientation data >> > + entry = av_dict_get(frame->metadata, "Orientation", NULL, 0); >> > + if (entry && entry->value) >> > + orientation = atoi(entry->value); >> > + >> > + // check frame orientation validity >> > + if (orientation < ORIENTATION_NORMAL || >> > + orientation > ORIENTATION_ROTATE270CW) { >> > + // frame orientation is invalid or empty, we have to guess one. >> > + av_log(ctx, AV_LOG_WARNING, "Frame orientation is either empty >> or >> > invalid.\n"); >> > + return s->orientation == ORIENTATION_AUTO_FLIP ? >> > + ORIENTATION_NORMAL : ORIENTATION_HFLIP_ROTATE270CW; >> > + } >> > + >> > + // check conflict between frame orientation and passed-in >> orientation >> > option >> > + if (s->orientation == ORIENTATION_AUTO_FLIP && >> > + orientation > ORIENTATION_VFLIP) { >> > + av_log(ctx, AV_LOG_WARNING, >> > + "Found valid orientation value: %i, but auto-flip only >> cover >> > range [1, 4]. Set to default value 1(ORIENTATION_NONE).\n", >> > + orientation); >> > + // conflict happens, return the min value of orientation range >> > + return ORIENTATION_NORMAL; >> > + } >> > + >> > + // check conflict between frame orientation and passed-in >> orientation >> > option >> > + if (s->orientation == ORIENTATION_AUTO_TRANSPOSE && >> > + orientation <= ORIENTATION_VFLIP) { >> > + av_log(ctx, AV_LOG_WARNING, >> > + "Found valid orientation value: %i, but auto-transpose >> only >> > cover range [5, 8]. Set to default value >> > 5(ORIENTATION_HFLIP_ROTATE270CW).\n", >> > + orientation); >> > + // conflict happens, return the min value of orientation range, >> > same effect as dir=0 >> > + return ORIENTATION_HFLIP_ROTATE270CW; >> > + } >> > + >> > + return orientation; >> > +} >> > + >> > +static int transpose_filter_slice(AVFilterContext *ctx, void *arg, int >> > jobnr, int nb_jobs) >> > { >> > TransContext *s = ctx->priv; >> > ThreadData *td = arg; >> > AVFrame *out = td->out; >> > AVFrame *in = td->in; >> > + TransContextData *c = &(s->ctx.transpose); >> > + int orientation = td->orientation; >> > int plane; >> > >> > - for (plane = 0; plane < s->planes; plane++) { >> > - int hsub = plane == 1 || plane == 2 ? s->hsub : 0; >> > - int vsub = plane == 1 || plane == 2 ? s->vsub : 0; >> > - int pixstep = s->pixsteps[plane]; >> > + for (plane = 0; plane < c->planes; plane++) { >> > + int hsub = plane == 1 || plane == 2 ? c->hsub : 0; >> > + int vsub = plane == 1 || plane == 2 ? c->vsub : 0; >> > + int pixstep = c->pixsteps[plane]; >> > int inh = AV_CEIL_RSHIFT(in->height, vsub); >> > int outw = AV_CEIL_RSHIFT(out->width, hsub); >> > int outh = AV_CEIL_RSHIFT(out->height, vsub); >> > @@ -285,19 +344,36 @@ static int filter_slice(AVFilterContext *ctx, void >> > *arg, int jobnr, >> > uint8_t *dst, *src; >> > int dstlinesize, srclinesize; >> > int x, y; >> > - TransVtable *v = &s->vtables[plane]; >> > + int dir = s->dir; >> > + TransVtable *v = &c->vtables[plane]; >> > >> > dstlinesize = out->linesize[plane]; >> > dst = out->data[plane] + start * dstlinesize; >> > src = in->data[plane]; >> > srclinesize = in->linesize[plane]; >> > + switch (orientation) { >> > + case ORIENTATION_HFLIP_ROTATE270CW: >> > + dir = 0; // mirror horizontal and rotate 270 CW, same >> > as >> > dir=cclock_flip >> > + break; >> > + case ORIENTATION_ROTATE90CW: >> > + dir = 1; // rotate 90 CW, same as dir=clock >> > + break; >> > + case ORIENTATION_HFLIP_ROTATE90CW: >> > + dir = 3; // mirror horizontal and rotate 90 CW, same as >> > dir=clock_flip >> > + break; >> > + case ORIENTATION_ROTATE270CW: >> > + dir = 2; // rotate 270 CW, same as dir=cclock >> > + break; >> > + default: >> > + break; >> > + } >> > >> > - if (s->dir & 1) { >> > + if (dir & 1) { >> > src += in->linesize[plane] * (inh - 1); >> > srclinesize *= -1; >> > } >> > >> > - if (s->dir & 2) { >> > + if (dir & 2) { >> > dst = out->data[plane] + dstlinesize * (outh - >> start - >> > 1); >> > dstlinesize *= -1; >> > } >> > @@ -326,6 +402,22 @@ static int filter_slice(AVFilterContext *ctx, void >> > *arg, int jobnr, >> > return 0; >> > } >> > >> > +static int filter_slice(AVFilterContext *ctx, void *arg, int jobnr, >> > + int nb_jobs) >> > +{ >> > + TransContext *s = ctx->priv; >> > + ThreadData *td = arg; >> > + AVFrame *out = td->out; >> > + AVFrame *in = td->in; >> > + int orientation = td->orientation; >> > + >> > + if (orientation > ORIENTATION_NONE && orientation <= >> ORIENTATION_VFLIP) >> > { >> > + return ff_hflip_filter_slice(&s->ctx.flip, in, out, jobnr, >> nb_jobs, >> > + orientation == >> ORIENTATION_ROTATE180); >> > + } >> > + return transpose_filter_slice(ctx, arg, jobnr, nb_jobs); >> > +} >> > + >> > static int filter_frame(AVFilterLink *inlink, AVFrame *in) >> > { >> > AVFilterContext *ctx = inlink->dst; >> > @@ -333,10 +425,29 @@ static int filter_frame(AVFilterLink *inlink, >> AVFrame >> > *in) >> > AVFilterLink *outlink = ctx->outputs[0]; >> > ThreadData td; >> > AVFrame *out; >> > + int orientation = ORIENTATION_NONE; >> > >> > if (s->passthrough) >> > return ff_filter_frame(outlink, in); >> > >> > + if (s->orientation) { >> > + orientation = get_frame_orientation(ctx, in); >> > + if (orientation == ORIENTATION_NORMAL) // orientation 'Normal', >> do >> > nothing >> > + return ff_filter_frame(outlink, in); >> > + >> > + if (orientation == ORIENTATION_VFLIP) { >> > + int i; >> > + for (i = 0; i < 4; i ++) { >> > + int height = s->ctx.flip.planeheight[i]; >> > + if (in->data[i]) { >> > + in->data[i] += (height - 1) * in->linesize[i]; >> > + in->linesize[i] = -in->linesize[i]; >> > + } >> > + } >> > + return ff_filter_frame(outlink, in); >> > + } >> > + } >> > + >> > out = ff_get_video_buffer(outlink, outlink->w, outlink->h); >> > if (!out) { >> > av_frame_free(&in); >> > @@ -346,12 +457,14 @@ static int filter_frame(AVFilterLink *inlink, >> AVFrame >> > *in) >> > >> > if (in->sample_aspect_ratio.num == 0) { >> > out->sample_aspect_ratio = in->sample_aspect_ratio; >> > - } else { >> > + } else if (s->orientation == ORIENTATION_AUTO_TRANSPOSE || >> > + orientation > ORIENTATION_VFLIP) { // transpose >> > out->sample_aspect_ratio.num = in->sample_aspect_ratio.den; >> > out->sample_aspect_ratio.den = in->sample_aspect_ratio.num; >> > } >> > >> > td.in = in, td.out = out; >> > + td.orientation = orientation; >> > ctx->internal->execute(ctx, filter_slice, &td, NULL, >> FFMIN(outlink->h, >> > ff_filter_get_nb_threads(ctx))); >> > av_frame_free(&in); >> > return ff_filter_frame(outlink, out); >> > @@ -373,6 +486,19 @@ static const AVOption transpose_options[] = { >> > { "portrait", "preserve portrait geometry", 0, >> > AV_OPT_TYPE_CONST, {.i64=TRANSPOSE_PT_TYPE_PORTRAIT}, INT_MIN, INT_MAX, >> > FLAGS, "passthrough" }, >> > { "landscape", "preserve landscape geometry", 0, >> > AV_OPT_TYPE_CONST, {.i64=TRANSPOSE_PT_TYPE_LANDSCAPE}, INT_MIN, INT_MAX, >> > FLAGS, "passthrough" }, >> > >> > + { "orientation", "apply frame exif orientation", >> > + OFFSET(orientation), AV_OPT_TYPE_INT, {.i64 = ORIENTATION_NONE }, >> > -2, 8, FLAGS, "orientation" }, >> >> -2 and 8 are still hardcoded here. > > > Hi Paul, > I actually explained this in path version 6 and thought it was fine since > no objection. > "I updated the default value type to enum. But I still keep the value range > -2 and 8 as numeric here since I see other options are doing that. Hope it > is fine." > I see other options in the same file like "dir" is doing that, maybe > consistent with them ? what do you think ?
Unacceptable explanation. The code is wrong. Making it consistent with wrong code will not make it better. > > Best Regards, > Jun > > >> >> + { "auto_transpose", "apply transposition if frame's >> orientation >> > value is in range [5,8]", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_AUTO_TRANSPOSE}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "auto_flip", "apply flip if frame's orientation value >> is >> > in range [1,4]", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_AUTO_FLIP}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "none", "apply default transposition, same as >> > dir=cclock_flip", 0, >> > AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_NONE}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "normal", "exif orientation 'Normal', do not apply >> any >> > flip or transposition", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_NORMAL}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "hflip", "exif orientation 'Mirror horizontal', >> apply >> > horizontal flip", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_HFLIP}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "rotate180", "exif orientation 'Rotate 180', apply >> > rotation 180", 0, >> > AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_ROTATE180}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "vflip", "exif orientation 'Mirror vertical', >> apply >> > vertical flip", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_VFLIP}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "hflip_rotate270cw", "exif orientation 'Mirror horizontal and >> > rotate 270 CW', same effect as dir=cclock_flip", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_HFLIP_ROTATE270CW}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "rotate90cw", "exif orientation 'Rotate 90 CW', same >> > as >> > dir=clock", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_ROTATE90CW}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "hflip_rotate90cw", "exif orientation 'Mirror horizontal and >> > rotate 90 CW', same effect as dir=clock_flip", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_HFLIP_ROTATE90CW}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > + { "rotate270", "exif orientation 'Rotate 270 CW', same >> as >> > dir=cclock", 0, AV_OPT_TYPE_CONST, >> > {.i64=ORIENTATION_ROTATE270CW}, INT_MIN, INT_MAX, FLAGS, >> "orientation" >> > }, >> > { NULL } >> > }; >> > >> > -- >> > 2.17.1 >> > >> > _______________________________________________ >> > 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". > _______________________________________________ > 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".