Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
On 9/4/2018 3:54 PM, Moritz Barsnick wrote:
On Tue, Sep 04, 2018 at 07:43:10 +, Roman Arzumanyan wrote:
Hi Timo,
Refactored the patch according to your review:
+{ "interp_algo","Interpolation algorithm used for transposing",
OFFSET(interp_algo), AV_OPT_TYPE_INT,{ .i64 = NPPI_INTER_CUBIC }, 0, INT_MAX, FLAGS,
"interp_algo" },
+{ "nn", "nearest neighbour",0,
AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_NN}, 0, 0, FLAGS, "interp_algo" },
+{ "linear", "linear", 0,
AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_LINEAR}, 0, 0, FLAGS, "interp_algo" },
+{ "cubic", "cubic",0,
AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_CUBIC }, 0, 0, FLAGS, "interp_algo" },
Shouldn't there be a range check somewhere? You allow INT_MAX (could be
NPPI_INTER_NN .. NPPI_INTER_CUBIC, assuming they are contiguous), and
pass the value directly to nppiRotate_8u_C1R(). I can't tell whether
that function does a proper check and error indication though, which
could suffice.
Interpolation also seems a bit weird in general for perfect 90°
rotations, they should always be pixel perfect no matter the algorithm.
smime.p7s
Description: S/MIME Cryptographic Signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
On Tue, Sep 04, 2018 at 07:43:10 +, Roman Arzumanyan wrote:
> Hi Timo,
>
> Refactored the patch according to your review:
> +{ "interp_algo","Interpolation algorithm used for transposing",
> OFFSET(interp_algo), AV_OPT_TYPE_INT,{ .i64 = NPPI_INTER_CUBIC }, 0,
> INT_MAX, FLAGS, "interp_algo" },
> +{ "nn", "nearest neighbour",0,
> AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_NN}, 0, 0,
> FLAGS, "interp_algo" },
> +{ "linear", "linear", 0,
> AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_LINEAR}, 0, 0,
> FLAGS, "interp_algo" },
> +{ "cubic", "cubic",0,
> AV_OPT_TYPE_CONST, { .i64 = NPPI_INTER_CUBIC }, 0, 0,
> FLAGS, "interp_algo" },
Shouldn't there be a range check somewhere? You allow INT_MAX (could be
NPPI_INTER_NN .. NPPI_INTER_CUBIC, assuming they are contiguous), and
pass the value directly to nppiRotate_8u_C1R(). I can't tell whether
that function does a proper check and error indication though, which
could suffice.
Cheers,
Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
Hi Timo, Refactored the patch according to your review: 1. It actually does the rotation, so renamed the filter. 2. Replaced "t" with the "a" & "angle" cli options like in vf_rotate filter. Though supporting rotation angle multiple of 90 degrees. 3. Added the config dependency (not sure if I've done it the right way). Cmd to check: ffmpeg.exe ^ -hwaccel cuvid ^ -codec:v h264_cuvid ^ -i input_240.mp4 ^ -vcodec h264_nvenc ^ -vf rotate_npp="a=180" ^ -y rotate_npp_180.mp4 -Original Message- From: ffmpeg-devel On Behalf Of Timo Rothenpieler Sent: Monday, September 3, 2018 1:43 PM To: FFmpeg development discussions and patches ; Roman Arzumanyan Cc: Yogender Gupta Subject: Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter Minus the missing configure dependency, documentation entry and minor bump, the filter looks fine to me code wise. Two issues though: From my understanding, it's not a transpose filter, but a rotation one, fixed to 90 degree angles. Unless I'm missing something in the code, it's not doing any actual flipping, like vf_transpose does. It should also be calling nppiTranspose_8u_C1R somewhere after/before the call to nppiRotate_8u_C1R. Or use a transformation matrix to do the transposition in one step, assuming libnpp has a function for that. It's possible that I missed it doing the transpose somewhere, so please correct me if I'm wrong. I'd also strongly prefer this filter to be compatible with the arguments of normal vf_transpose, and behave the same for the same arguments. Additional parameters with more functions are fine. Timo --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- 0001-Adding-NPP-rotate-filter.patch Description: 0001-Adding-NPP-rotate-filter.patch ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
I did some re-formating and added the missing configure bit. Updated patch for that can be found on Github: https://github.com/BtbN/FFmpeg/commit/3e9ad52cfbebfade0d26758dad862e0c0a002c91 https://github.com/BtbN/FFmpeg/commit/3e9ad52cfbebfade0d26758dad862e0c0a002c91.patch smime.p7s Description: S/MIME Cryptographic Signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
Minus the missing configure dependency, documentation entry and minor bump, the filter looks fine to me code wise. Two issues though: From my understanding, it's not a transpose filter, but a rotation one, fixed to 90 degree angles. Unless I'm missing something in the code, it's not doing any actual flipping, like vf_transpose does. It should also be calling nppiTranspose_8u_C1R somewhere after/before the call to nppiRotate_8u_C1R. Or use a transformation matrix to do the transposition in one step, assuming libnpp has a function for that. It's possible that I missed it doing the transpose somewhere, so please correct me if I'm wrong. I'd also strongly prefer this filter to be compatible with the arguments of normal vf_transpose, and behave the same for the same arguments. Additional parameters with more functions are fine. Timo smime.p7s Description: S/MIME Cryptographic Signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
On Thu, Aug 30, 2018 at 10:13:28AM +, Roman Arzumanyan wrote: > Hello, > This patch adds NPP transpose filter. > > Cmd example: > ffmpeg.exe -hwaccel cuvid -c:v h264_cuvid -i input.mp4 -vf > transpose_npp="t=3" -c:v h264_nvenc -y output_transpose_npp.mp4 > > Supported values: > t=0 no transpose > t=1 90 deg clockwise > t=2 180 deg clockwise > t=3 270 deg clockwise > > -- > BR, Roman Arzumanyan > > > --- > This email message is for the sole use of the intended recipient(s) and may > contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact the > sender by > reply email and destroy all copies of the original message. > --- > Makefile |1 > allfilters.c |1 > vf_transpose_npp.c | 515 > + > 3 files changed, 517 insertions(+) > 3abd340cf177c125c57233d0085ab794e86fbf09 > 0001-Adding-NPP-transpose-filter.patch > From 72ce447ea2bf3c0fbf98a3324b56c1875d3dcb5f Mon Sep 17 00:00:00 2001 > From: Roman Arzumanyan > Date: Wed, 29 Aug 2018 12:53:02 +0300 > Subject: [PATCH] Adding NPP transpose filter this breaks build: make distclean ; ./configure && make -j12 CC libavfilter/vf_transpose_npp.o libavfilter/vf_transpose_npp.c:22:18: fatal error: nppi.h: No such file or directory #include ^ compilation terminated. make: *** [libavfilter/vf_transpose_npp.o] Error 1 make: Target `all' not remade because of errors. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB "I am not trying to be anyone's saviour, I'm trying to think about the future and not be sad" - Elon Musk signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avfilter/vf_transpose adding NPP transpose filter
Thanks! I'm a bit short on time at the moment, but I'll try to have a look during the weekend. If someone else also wants to give a review, please go ahead! smime.p7s Description: S/MIME Cryptographic Signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
