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:
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".
