diff mbox

[FFmpeg-devel,1/2] avfilter/vf_histogram: clean up code

Message ID 20191213135936.87027-1-quinkblack@foxmail.com
State New
Headers show

Commit Message

Zhao Zhili Dec. 13, 2019, 1:59 p.m. UTC 
From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavfilter/vf_histogram.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Paul B Mahol Dec. 13, 2019, 2 p.m. UTC | #1 
Please provide some explanation.

On 12/13/19, quinkblack@foxmail.com <quinkblack@foxmail.com> wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavfilter/vf_histogram.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/libavfilter/vf_histogram.c b/libavfilter/vf_histogram.c
> index 5185992de6..0d2d087beb 100644
> --- a/libavfilter/vf_histogram.c
> +++ b/libavfilter/vf_histogram.c
> @@ -266,20 +266,20 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in)
>          const int is_chroma = (k == 1 || k == 2);
>          const int dst_h = AV_CEIL_RSHIFT(outlink->h, (is_chroma ?
> h->odesc->log2_chroma_h : 0));
>          const int dst_w = AV_CEIL_RSHIFT(outlink->w, (is_chroma ?
> h->odesc->log2_chroma_w : 0));
> +        const int plane = h->odesc->comp[k].plane;
> +        uint8_t *const data = out->data[plane];
> +        const int linesize = out->linesize[plane];
>
>          if (h->histogram_size <= 256) {
>              for (i = 0; i < dst_h ; i++)
> -                memset(out->data[h->odesc->comp[k].plane] +
> -                       i * out->linesize[h->odesc->comp[k].plane],
> -                       h->bg_color[k], dst_w);
> +                memset(data + i * linesize, h->bg_color[k], dst_w);
>          } else {
>              const int mult = h->mult;
>
> -            for (i = 0; i < dst_h ; i++)
> -                for (j = 0; j < dst_w; j++)
> -                    AV_WN16(out->data[h->odesc->comp[k].plane] +
> -                        i * out->linesize[h->odesc->comp[k].plane] + j * 2,
> -                        h->bg_color[k] * mult);
> +            for (j = 0; j < dst_w; j++)
> +                AV_WN16(data + j * 2, h->bg_color[k] * mult);
> +            for (i = 1; i < dst_h; i++)
> +                memcpy(data + i * linesize, data, dst_w * 2);
>          }
>      }
>
> --
> 2.22.0
>
>
>
> _______________________________________________
> 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".
Zhao Zhili Dec. 14, 2019, 7:28 a.m. UTC | #2 
Hi Paul,

> On Dec 13, 2019, at 10:00 PM, Paul B Mahol <onemda@gmail.com> wrote:
> 
> Please provide some explanation.

No functional changes, just use temporary variables to make code more readable,
and may improve a little bit of performance.

> 
> On 12/13/19, quinkblack@foxmail.com <quinkblack@foxmail.com> wrote:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> libavfilter/vf_histogram.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavfilter/vf_histogram.c b/libavfilter/vf_histogram.c
>> index 5185992de6..0d2d087beb 100644
>> --- a/libavfilter/vf_histogram.c
>> +++ b/libavfilter/vf_histogram.c
>> @@ -266,20 +266,20 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
>> *in)
>>         const int is_chroma = (k == 1 || k == 2);
>>         const int dst_h = AV_CEIL_RSHIFT(outlink->h, (is_chroma ?
>> h->odesc->log2_chroma_h : 0));
>>         const int dst_w = AV_CEIL_RSHIFT(outlink->w, (is_chroma ?
>> h->odesc->log2_chroma_w : 0));
>> +        const int plane = h->odesc->comp[k].plane;
>> +        uint8_t *const data = out->data[plane];
>> +        const int linesize = out->linesize[plane];
>> 
>>         if (h->histogram_size <= 256) {
>>             for (i = 0; i < dst_h ; i++)
>> -                memset(out->data[h->odesc->comp[k].plane] +
>> -                       i * out->linesize[h->odesc->comp[k].plane],
>> -                       h->bg_color[k], dst_w);
>> +                memset(data + i * linesize, h->bg_color[k], dst_w);
>>         } else {
>>             const int mult = h->mult;
>> 
>> -            for (i = 0; i < dst_h ; i++)
>> -                for (j = 0; j < dst_w; j++)
>> -                    AV_WN16(out->data[h->odesc->comp[k].plane] +
>> -                        i * out->linesize[h->odesc->comp[k].plane] + j * 2,
>> -                        h->bg_color[k] * mult);
>> +            for (j = 0; j < dst_w; j++)
>> +                AV_WN16(data + j * 2, h->bg_color[k] * mult);
>> +            for (i = 1; i < dst_h; i++)
>> +                memcpy(data + i * linesize, data, dst_w * 2);
>>         }
>>     }
>> 
>> --
>> 2.22.0
>> 
>> 
>> 
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
Carl Eugen Hoyos Dec. 14, 2019, 4:35 p.m. UTC | #3 
Am Sa., 14. Dez. 2019 um 08:28 Uhr schrieb zhilizhao <quinkblack@foxmail.com>:

> > On Dec 13, 2019, at 10:00 PM, Paul B Mahol <onemda@gmail.com> wrote:
> >
> > Please provide some explanation.
>
> No functional changes, just use temporary variables to make code more readable,

> and may improve a little bit of performance.

Such a claim needs numbers and / or a testcase.

Carl Eugen
Zhao Zhili Dec. 16, 2019, 8:05 a.m. UTC | #4 
> On Dec 15, 2019, at 12:35 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> Am Sa., 14. Dez. 2019 um 08:28 Uhr schrieb zhilizhao <quinkblack@foxmail.com>:
> 
>>> On Dec 13, 2019, at 10:00 PM, Paul B Mahol <onemda@gmail.com> wrote:
>>> 
>>> Please provide some explanation.
>> 
>> No functional changes, just use temporary variables to make code more readable,
> 
>> and may improve a little bit of performance.
> 
> Such a claim needs numbers and / or a testcase.

It may or may not improve performance depends on compiler optimization, and I don’t
think such small change can lead to measurable difference.

Assemble results on aarch64. Less deferences lead to smaller inner loop, but it doesn’t
mean much.


Before:
.LBB1_11:                               //   Parent Loop BB1_4 Depth=1
                                        // =>  This Inner Loop Header: Depth=2
	//DEBUG_VALUE: filter_frame:i <- 0
	//DEBUG_VALUE: dst_w <- $w11
	//DEBUG_VALUE: dst_h <- $w25
	//DEBUG_VALUE: filter_frame:k <- $x19
	//DEBUG_VALUE: filter_frame:out <- $x20
	//DEBUG_VALUE: filter_frame:outlink <- $x28
	//DEBUG_VALUE: filter_frame:h <- $x22
	//DEBUG_VALUE: filter_frame:i <- $w26
	.loc	15 272 37 is_stmt 1     // src/libavfilter/vf_histogram.c:272:37
	ldr	x8, [x23, #48]
	.loc	15 272 17 is_stmt 0     // src/libavfilter/vf_histogram.c:272:17
	ldrb	w1, [x27]
	.loc	15 272 59               // src/libavfilter/vf_histogram.c:272:59
	sxtw	x10, w26
	.loc	15 272 17               // src/libavfilter/vf_histogram.c:272:17
	mov	x2, x21
	.loc	15 272 52               // src/libavfilter/vf_histogram.c:272:52
	add	x8, x8, x19, lsl #5
	ldrsw	x8, [x8, #24]
	.loc	15 272 24               // src/libavfilter/vf_histogram.c:272:24
	ldr	x9, [x20, x8, lsl #3]
	.loc	15 273 28 is_stmt 1     // src/libavfilter/vf_histogram.c:273:28
	add	x8, x20, x8, lsl #2
	ldrsw	x8, [x8, #64]
	.loc	15 272 59               // src/libavfilter/vf_histogram.c:272:59
	nop
	madd	x0, x8, x10, x9
	.loc	15 272 17 is_stmt 0     // src/libavfilter/vf_histogram.c:272:17
	bl	memset
.Ltmp74:
	.loc	15 271 38 is_stmt 1     // src/libavfilter/vf_histogram.c:271:38
	add	w26, w26, #1            // =1
.Ltmp75:
	//DEBUG_VALUE: filter_frame:i <- $w26
	.loc	15 271 13 is_stmt 0     // src/libavfilter/vf_histogram.c:271:13
	cmp	w25, w26
	b.ne	.LBB1_11
-----------------------------------------------------------------------------------
After:
.LBB1_11:                               //   Parent Loop BB1_4 Depth=1
                                        // =>  This Inner Loop Header: Depth=2
	//DEBUG_VALUE: dst_h <- $w29
	//DEBUG_VALUE: filter_frame:outlink <- [DW_OP_plus_uconst 16] [$sp+0]
	//DEBUG_VALUE: filter_frame:i <- 0
	//DEBUG_VALUE: data <- $x21
	//DEBUG_VALUE: dst_w <- $w9
	//DEBUG_VALUE: filter_frame:k <- $x19
	//DEBUG_VALUE: filter_frame:out <- $x20
	//DEBUG_VALUE: filter_frame:h <- $x23
	//DEBUG_VALUE: filter_frame:i <- undef
	.loc	15 275 17 is_stmt 1     // src/libavfilter/vf_histogram.c:275:17
	ldrb	w1, [x28]
	mov	x0, x21
	mov	x2, x22
	bl	memset
.Ltmp79:
	//DEBUG_VALUE: filter_frame:i <- [DW_OP_plus_uconst 1, DW_OP_stack_value] undef
	.loc	15 274 27               // src/libavfilter/vf_histogram.c:274:27
	subs	x29, x29, #1            // =1
.Ltmp80:
	add	x21, x21, x27
.Ltmp81:
	.loc	15 274 13 is_stmt 0     // src/libavfilter/vf_histogram.c:274:13
	b.ne	.LBB1_11


> 
> Carl Eugen
> _______________________________________________
> 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".
Carl Eugen Hoyos Dec. 16, 2019, 10:47 a.m. UTC | #5 
Am Mo., 16. Dez. 2019 um 09:05 Uhr schrieb zhilizhao <quinkblack@foxmail.com>:
>
> > On Dec 15, 2019, at 12:35 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > Am Sa., 14. Dez. 2019 um 08:28 Uhr schrieb zhilizhao <quinkblack@foxmail.com>:

> >> and may improve a little bit of performance.
> >
> > Such a claim needs numbers and / or a testcase.
>
> It may or may not improve performance depends on compiler optimization, and
> I don’t think such small change can lead to measurable difference.

Then please don't claim it.

I don't know if your patch improves performance or not, it would be great
if it does.

Carl Eugen
diff mbox

Patch

diff --git a/libavfilter/vf_histogram.c b/libavfilter/vf_histogram.c
index 5185992de6..0d2d087beb 100644
--- a/libavfilter/vf_histogram.c
+++ b/libavfilter/vf_histogram.c
@@ -266,20 +266,20 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *in)
         const int is_chroma = (k == 1 || k == 2);
         const int dst_h = AV_CEIL_RSHIFT(outlink->h, (is_chroma ? h->odesc->log2_chroma_h : 0));
         const int dst_w = AV_CEIL_RSHIFT(outlink->w, (is_chroma ? h->odesc->log2_chroma_w : 0));
+        const int plane = h->odesc->comp[k].plane;
+        uint8_t *const data = out->data[plane];
+        const int linesize = out->linesize[plane];
 
         if (h->histogram_size <= 256) {
             for (i = 0; i < dst_h ; i++)
-                memset(out->data[h->odesc->comp[k].plane] +
-                       i * out->linesize[h->odesc->comp[k].plane],
-                       h->bg_color[k], dst_w);
+                memset(data + i * linesize, h->bg_color[k], dst_w);
         } else {
             const int mult = h->mult;
 
-            for (i = 0; i < dst_h ; i++)
-                for (j = 0; j < dst_w; j++)
-                    AV_WN16(out->data[h->odesc->comp[k].plane] +
-                        i * out->linesize[h->odesc->comp[k].plane] + j * 2,
-                        h->bg_color[k] * mult);
+            for (j = 0; j < dst_w; j++)
+                AV_WN16(data + j * 2, h->bg_color[k] * mult);
+            for (i = 1; i < dst_h; i++)
+                memcpy(data + i * linesize, data, dst_w * 2);
         }
     }