[libav-devel] [PATCH 2/2] h263dsp: K&R formatting cosmetics

Diego Biurrun diego at biurrun.de
Thu Nov 7 14:58:02 CET 2013


On Thu, Nov 07, 2013 at 03:29:24PM +0200, Martin Storsjö wrote:
> On Mon, 4 Nov 2013, Diego Biurrun wrote:
> >--- a/libavcodec/h263dsp.c
> >+++ b/libavcodec/h263dsp.c
> >@@ -23,78 +23,94 @@
> >#include "config.h"
> >#include "h263dsp.h"
> >
> >-const uint8_t ff_h263_loop_filter_strength[32]={
> >-//  0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> >-    0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 6, 6, 7, 7, 7, 8, 8, 8, 9, 9, 9,10,10,10,11,11,11,12,12,12
> 
> This also removes some commented out code - that's maybe a part of a
> general cosmetic cleanup, but when the topic is "K&R formatting
> cosmetics" I would expect it to be a pure reindentation patch,
> because that's the only thing that K&R says anything about at all.
> 
> Please at least mention this in the commit message in some way.
> (Ideally please also dig through the git history to figure out what
> these commented out coefficientes are and why they were kept to
> begin with and mention that in the commit message.)

It's been there since the beginning.  IMO these are not coefficients,
just a numbering of the array elements from 0 - 31.

If you look at h263data.h, you will see more instances, none were
changed since they were introduced.  Would you be happy with either of

"Also remove some silly comments."
"Also remove array element numbering comments."

added to the log message?

> >-static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale){
> >+static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
> >+{
> >    int x;
> >-    const int strength= ff_h263_loop_filter_strength[qscale];
> >+    const int strength = ff_h263_loop_filter_strength[qscale];
> >
> >-    for(x=0; x<8; x++){
> >+    for (x = 0; x < 8; x++) {
> >        int d1, d2, ad1;
> >-        int p0= src[x-2*stride];
> >-        int p1= src[x-1*stride];
> >-        int p2= src[x+0*stride];
> >-        int p3= src[x+1*stride];
> >-        int d = (p0 - p3 + 4*(p2 - p1)) / 8;
> >-
> >-        if     (d<-2*strength) d1= 0;
> >-        else if(d<-  strength) d1=-2*strength - d;
> >-        else if(d<   strength) d1= d;
> >-        else if(d< 2*strength) d1= 2*strength - d;
> >-        else                   d1= 0;
> >+        int p0 = src[x - 2 * stride];
> >+        int p1 = src[x - 1 * stride];
> >+        int p2 = src[x + 0 * stride];
> >+        int p3 = src[x + 1 * stride];
> >+        int d  = (p0 - p3 + 4 * (p2 - p1)) / 8;
> >+
> >+        if (d < -2 * strength)
> >+            d1 = 0;
> >+        else if (d < -strength)
> >+            d1 = -2 * strength - d;
> >+        else if (d < strength)
> >+            d1 = d;
> >+        else if (d < 2 * strength)
> >+            d1 = 2 * strength - d;
> >+        else
> >+            d1 = 0;
> 
> Some might argue that this was more readable in the previous form,
> but I'm ok with it in this form as well, and this obviously matches
> the general formatting style closer. (Keeping things on one line is
> ok with me in the odd cases where it significantly helps readability
> - in this case I'm not sure how significantly it helps.)

IMO this hardly helps here, so I decided against idiosyncratic formatting.

Diego


More information about the libav-devel mailing list