On 5/27/19, Jun Li <junli1...@gmail.com> wrote: > On Sun, May 26, 2019 at 2:09 AM Paul B Mahol <one...@gmail.com> wrote: > >> On 5/26/19, Jun Li <junli1...@gmail.com> wrote: >> > On Sun, May 26, 2019 at 1:16 AM Paul B Mahol <one...@gmail.com> wrote: >> > >> >> On 5/25/19, Jun Li <junli1...@gmail.com> wrote: >> >> > Add exif orientation support and expose an option. >> >> > --- >> >> > libavfilter/vf_transpose.c | 258 >> +++++++++++++++++++++++++++++-------- >> >> > 1 file changed, 207 insertions(+), 51 deletions(-) >> >> > >> >> > diff --git a/libavfilter/vf_transpose.c b/libavfilter/vf_transpose.c >> >> > index dd54947bd9..4aebfb9ee4 100644 >> >> > --- a/libavfilter/vf_transpose.c >> >> > +++ b/libavfilter/vf_transpose.c >> >> > @@ -46,6 +46,9 @@ typedef struct TransVtable { >> >> > void (*transpose_block)(uint8_t *src, ptrdiff_t src_linesize, >> >> > uint8_t *dst, ptrdiff_t dst_linesize, >> >> > int w, int h); >> >> > + void (*copyline_block)(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h); >> >> > } TransVtable; >> >> > >> >> >> >> This is slow, the operations should be in one loop. >> >> >> > >> > Thanks Paul for review. >> > I see transpose is using " ctx->internal->execute(ctx, filter_slice, >> > &td, >> > NULL, FFMIN(outlink->h, ff_filter_get_nb_threads(ctx)));" . >> > which looks like leveraging multi-threading. >> > If we do that in one loop, I guess it would be hard to use >> multi-threading >> > to accelerate ? But correct me if I am wrong. >> >> Transpose filter does one thing and that is transposing. >> >> If I'm not mistaken you are adding vertical and horizontal flipping? >> > > Thanks for review. > Yes, I am doing vflip, hflip and rotate180(vflip+hflip). > > >> If you really need to do that, do it efficiently in one loop, by that I >> mean >> you do not do cascading operations at all. >> > > Are you suggesting not use function pointer, and call the function directly > like this:
Please do not invent things I never said. I think I was very clear. But to repeat it once more, it is unacceptable to transpose and do vflip/hflip as two separate operations. Instead everything should be done in single pass. > switch (s->pixsteps[plane]) { > case 1: copyline_block_8(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > case 2: copyline_block_16(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > case 3: copyline_block_24(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > case 4: copyline_block_32(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > case 6: copyline_block_48(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > case 8: copyline_block_64(src, srclinesize, dst, > dstlinesize, line_dir, outw, end - start); break; > } > > I did some perf comparsion, they are very close and no significant > difference, about 9ms for one frame. > > Also I did some comparison between current hflip filter and this patch, > they take almost the same time, about 9ms too on my machine and I see 13 > threads/jobs are used for filtering. > I just use the simple command "ffmpeg -i test.bmp -vf > transpose=orientation=2 -y hello.jpg". > Current vflip wins a lot and I see it is not copying frame at all, just > like "orientation=1" or "passthrough" cases. > > I hope I understand you correctly, especially about "cascading operations". > But correct me if I am wrong. > Thanks for your time Paul. > > Best Regards, > -Jun > >> >> > Best Regards, >> > -Jun >> > >> > >> >> > typedef struct TransContext { >> >> > @@ -56,7 +59,7 @@ typedef struct TransContext { >> >> > >> >> > int passthrough; ///< PassthroughType, landscape passthrough >> >> > mode >> >> > enabled >> >> > int dir; ///< TransposeDir >> >> > - >> >> > + int orientation; ///< Orientation >> >> > TransVtable vtables[4]; >> >> > } TransContext; >> >> > >> >> > @@ -182,6 +185,105 @@ static void transpose_8x8_64_c(uint8_t *src, >> >> ptrdiff_t >> >> > src_linesize, >> >> > transpose_block_64_c(src, src_linesize, dst, dst_linesize, 8, >> >> > 8); >> >> > } >> >> > >> >> > + >> >> > +static void copyline_block_8(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x ++) { >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + dst[i] = src[x]; >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void copyline_block_16(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x ++) { >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + *((uint16_t *)(dst + 2*i)) = *((uint16_t *)(src + 2*x)); >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void copyline_block_24(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x++) { >> >> > + int32_t v = AV_RB24(src + 3*x); >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + AV_WB24(dst + 3*i, v); >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void copyline_block_32(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x ++) { >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + *((uint32_t *)(dst + 4*i)) = *((uint32_t *)(src + 4*x)); >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void copyline_block_48(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x++) { >> >> > + int64_t v = AV_RB48(src + 6*x); >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + AV_WB48(dst + 6*i, v); >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void copyline_block_64(uint8_t *src, ptrdiff_t src_linesize, >> >> > + uint8_t *dst, ptrdiff_t dst_linesize, >> >> > + int line_dir, int w, int h) >> >> > +{ >> >> > + int x, y, i; >> >> > + for (y = 0; y < h; y++, dst += dst_linesize, src += >> src_linesize) { >> >> > + for (x = 0; x < w; x ++) { >> >> > + i = line_dir < 0 ? w-x-1 : x; >> >> > + *((uint64_t *)(dst + 8*i)) = *((uint64_t *)(src + 8*x)); >> >> > + } >> >> > + } >> >> > +} >> >> > + >> >> > +static void set_outlink_width_height(AVFilterLink *inlink, >> AVFilterLink >> >> > *outlink, int transpose) >> >> > +{ >> >> > + if (transpose) { >> >> > + outlink->w = inlink->h; >> >> > + outlink->h = inlink->w; >> >> > + >> >> > + if (inlink->sample_aspect_ratio.num) >> >> > + outlink->sample_aspect_ratio = av_div_q((AVRational) { >> 1, 1 >> >> }, >> >> > + >> >> > inlink->sample_aspect_ratio); >> >> > + else >> >> > + outlink->sample_aspect_ratio = >> inlink->sample_aspect_ratio; >> >> > + } else { >> >> > + outlink->w = inlink->w; >> >> > + outlink->h = inlink->h; >> >> > + outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; >> >> > + } >> >> > +} >> >> > + >> >> > static int config_props_output(AVFilterLink *outlink) >> >> > { >> >> > AVFilterContext *ctx = outlink->src; >> >> > @@ -213,33 +315,44 @@ static int config_props_output(AVFilterLink >> >> *outlink) >> >> > >> >> > av_assert0(desc_in->nb_components == desc_out->nb_components); >> >> > >> >> > - >> >> > av_image_fill_max_pixsteps(s->pixsteps, NULL, desc_out); >> >> > + >> >> > + if (s->orientation && s->orientation < 5) { >> >> > + outlink->h = inlink->h; >> >> > + outlink->w = inlink->w; >> >> > + outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; >> >> > + } else { >> >> > + outlink->w = inlink->h; >> >> > + outlink->h = inlink->w; >> >> > >> >> > - outlink->w = inlink->h; >> >> > - outlink->h = inlink->w; >> >> > - >> >> > - if (inlink->sample_aspect_ratio.num) >> >> > - outlink->sample_aspect_ratio = av_div_q((AVRational) { 1, 1 >> }, >> >> > - >> >> > inlink->sample_aspect_ratio); >> >> > - else >> >> > - outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; >> >> > + if (inlink->sample_aspect_ratio.num) >> >> > + outlink->sample_aspect_ratio = av_div_q((AVRational) { >> 1, 1 >> >> }, >> >> > + >> >> > inlink->sample_aspect_ratio); >> >> > + else >> >> > + outlink->sample_aspect_ratio = >> inlink->sample_aspect_ratio; >> >> > + } >> >> > >> >> > for (int i = 0; i < 4; i++) { >> >> > TransVtable *v = &s->vtables[i]; >> >> > switch (s->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; >> >> > + v->copyline_block = copyline_block_8; break; >> >> > case 2: v->transpose_block = transpose_block_16_c; >> >> > - v->transpose_8x8 = transpose_8x8_16_c; break; >> >> > + v->transpose_8x8 = transpose_8x8_16_c; >> >> > + v->copyline_block = copyline_block_16; break; >> >> > case 3: v->transpose_block = transpose_block_24_c; >> >> > - v->transpose_8x8 = transpose_8x8_24_c; break; >> >> > + v->transpose_8x8 = transpose_8x8_24_c; >> >> > + v->copyline_block = copyline_block_24; break; >> >> > case 4: v->transpose_block = transpose_block_32_c; >> >> > - v->transpose_8x8 = transpose_8x8_32_c; break; >> >> > + v->transpose_8x8 = transpose_8x8_32_c; >> >> > + v->copyline_block = copyline_block_32; break; >> >> > case 6: v->transpose_block = transpose_block_48_c; >> >> > - v->transpose_8x8 = transpose_8x8_48_c; break; >> >> > + v->transpose_8x8 = transpose_8x8_48_c; >> >> > + v->copyline_block = copyline_block_48; break; >> >> > case 8: v->transpose_block = transpose_block_64_c; >> >> > - v->transpose_8x8 = transpose_8x8_64_c; break; >> >> > + v->transpose_8x8 = transpose_8x8_64_c; >> >> > + v->copyline_block = copyline_block_64; break; >> >> > } >> >> > } >> >> > >> >> > @@ -285,42 +398,80 @@ static int filter_slice(AVFilterContext *ctx, >> void >> >> > *arg, int jobnr, >> >> > uint8_t *dst, *src; >> >> > int dstlinesize, srclinesize; >> >> > int x, y; >> >> > + int dir = s->dir; >> >> > TransVtable *v = &s->vtables[plane]; >> >> > + >> >> > + if (s->orientation > 0 && s->orientation < 5) { >> >> > + int line_dir = 1; >> >> > + dstlinesize = out->linesize[plane]; >> >> > + dst = out->data[plane] + start * dstlinesize; >> >> > + srclinesize = in->linesize[plane]; >> >> > + src = in->data[plane] + start * srclinesize; >> >> > + >> >> > + switch (s->orientation) { >> >> > + case 2: // mirror horizontal >> >> > + line_dir = -1; >> >> > + break; >> >> > + case 3: // rotate 180 >> >> > + dst = out->data[plane] + (outh - start >> >> > - >> >> 1) * >> >> > dstlinesize; >> >> > + line_dir = -1; >> >> > + dstlinesize *= -1; >> >> > + break; >> >> > + case 4: // mirror vertical >> >> > + dst = out->data[plane] + (outh - start >> >> > - >> >> 1) * >> >> > dstlinesize; >> >> > + dstlinesize *= -1; >> >> > + break; >> >> > + default: >> >> > + break; >> >> > + } >> >> > + v->copyline_block(src, srclinesize, dst, dstlinesize, >> >> line_dir, >> >> > outw, end - start); >> >> > + } else { >> >> > + dstlinesize = out->linesize[plane]; >> >> > + dst = out->data[plane] + start * dstlinesize; >> >> > + src = in->data[plane]; >> >> > + srclinesize = in->linesize[plane]; >> >> > + switch (s->orientation) { >> >> > + case 5: // mirror horizontal and rotate 270 CW >> >> > + dir = 0; break; >> >> > + case 6: // rotate 90 CW >> >> > + dir = 1; break; >> >> > + case 7: // mirror horizontal and rotate 90 CW >> >> > + dir = 3; break; >> >> > + case 8: // rotate 270 CW >> >> > + dir = 2; break; >> >> > + default: break; >> >> > + } >> >> > >> >> > - dstlinesize = out->linesize[plane]; >> >> > - dst = out->data[plane] + start * dstlinesize; >> >> > - src = in->data[plane]; >> >> > - srclinesize = in->linesize[plane]; >> >> > - >> >> > - if (s->dir & 1) { >> >> > - src += in->linesize[plane] * (inh - 1); >> >> > - srclinesize *= -1; >> >> > - } >> >> > + if (dir & 1) { >> >> > + src += in->linesize[plane] * (inh - 1); >> >> > + srclinesize *= -1; >> >> > + } >> >> > >> >> > - if (s->dir & 2) { >> >> > - dst = out->data[plane] + dstlinesize * (outh - >> >> start - >> >> > 1); >> >> > - dstlinesize *= -1; >> >> > - } >> >> > + if (dir & 2) { >> >> > + dst = out->data[plane] + dstlinesize * >> (outh - >> >> > start - 1); >> >> > + dstlinesize *= -1; >> >> > + } >> >> > >> >> > - for (y = start; y < end - 7; y += 8) { >> >> > - for (x = 0; x < outw - 7; x += 8) { >> >> > - v->transpose_8x8(src + x * srclinesize + y * >> >> > pixstep, >> >> > - srclinesize, >> >> > - dst + (y - start) * dstlinesize + x >> * >> >> > pixstep, >> >> > - dstlinesize); >> >> > + for (y = start; y < end - 7; y += 8) { >> >> > + for (x = 0; x < outw - 7; x += 8) { >> >> > + v->transpose_8x8(src + x * srclinesize + y * >> >> pixstep, >> >> > + srclinesize, >> >> > + dst + (y - start) * dstlinesize >> + x >> >> * >> >> > pixstep, >> >> > + dstlinesize); >> >> > + } >> >> > + if (outw - x > 0 && end - y > 0) >> >> > + v->transpose_block(src + x * srclinesize + y * >> >> pixstep, >> >> > + srclinesize, >> >> > + dst + (y - start) * dstlinesize >> + x >> >> * >> >> > pixstep, >> >> > + dstlinesize, outw - x, end - y); >> >> > } >> >> > - if (outw - x > 0 && end - y > 0) >> >> > - v->transpose_block(src + x * srclinesize + y * >> pixstep, >> >> > - srclinesize, >> >> > - dst + (y - start) * dstlinesize + >> x >> >> > * >> >> > pixstep, >> >> > - dstlinesize, outw - x, end - y); >> >> > - } >> >> > >> >> > - if (end - y > 0) >> >> > - v->transpose_block(src + 0 * srclinesize + y * pixstep, >> >> > - srclinesize, >> >> > - dst + (y - start) * dstlinesize + 0 * >> >> > pixstep, >> >> > - dstlinesize, outw, end - y); >> >> > + if (end - y > 0) >> >> > + v->transpose_block(src + 0 * srclinesize + y * >> pixstep, >> >> > + srclinesize, >> >> > + dst + (y - start) * dstlinesize + 0 >> >> > * >> >> > pixstep, >> >> > + dstlinesize, outw, end - y); >> >> > + } >> >> > } >> >> > >> >> > return 0; >> >> > @@ -334,7 +485,7 @@ static int filter_frame(AVFilterLink *inlink, >> >> > AVFrame >> >> > *in) >> >> > ThreadData td; >> >> > AVFrame *out; >> >> > >> >> > - if (s->passthrough) >> >> > + if (s->passthrough || s->orientation == 1) >> >> > return ff_filter_frame(outlink, in); >> >> > >> >> > out = ff_get_video_buffer(outlink, outlink->w, outlink->h); >> >> > @@ -347,8 +498,13 @@ static int filter_frame(AVFilterLink *inlink, >> >> AVFrame >> >> > *in) >> >> > if (in->sample_aspect_ratio.num == 0) { >> >> > out->sample_aspect_ratio = in->sample_aspect_ratio; >> >> > } else { >> >> > - out->sample_aspect_ratio.num = in->sample_aspect_ratio.den; >> >> > - out->sample_aspect_ratio.den = in->sample_aspect_ratio.num; >> >> > + if (s->orientation > 0 && s->orientation < 5) { >> >> > + out->sample_aspect_ratio.num = >> in->sample_aspect_ratio.num; >> >> > + out->sample_aspect_ratio.den = >> in->sample_aspect_ratio.den; >> >> > + } else { >> >> > + 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; >> >> > @@ -366,13 +522,13 @@ static const AVOption transpose_options[] = { >> >> > { "clock", "rotate clockwise", >> >> 0, >> >> > AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CLOCK }, .flags=FLAGS, >> .unit >> >> = >> >> > "dir" }, >> >> > { "cclock", "rotate counter-clockwise", >> >> 0, >> >> > AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CCLOCK }, .flags=FLAGS, >> .unit >> >> = >> >> > "dir" }, >> >> > { "clock_flip", "rotate clockwise with vertical flip", >> >> 0, >> >> > AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CLOCK_FLIP }, .flags=FLAGS, >> .unit >> >> = >> >> > "dir" }, >> >> > - >> >> > + >> >> > { "passthrough", "do not apply transposition if the input >> >> > matches >> >> the >> >> > specified geometry", >> >> > OFFSET(passthrough), AV_OPT_TYPE_INT, >> >> {.i64=TRANSPOSE_PT_TYPE_NONE}, >> >> > 0, INT_MAX, FLAGS, "passthrough" }, >> >> > { "none", "always apply transposition", 0, >> >> > AV_OPT_TYPE_CONST, {.i64=TRANSPOSE_PT_TYPE_NONE}, INT_MIN, >> INT_MAX, >> >> > FLAGS, "passthrough" }, >> >> > { "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", "set exif orientation", OFFSET(orientation), >> >> > AV_OPT_TYPE_INT, {.i64 = 0 }, 0, 8, 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". > _______________________________________________ > 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".