[vlc-devel] [Patch] New audio & video filters for audio bargraph display inside video

[jpd at videolan.org]

On Wed, Nov 18, 2009 at 03:33:17PM +0100, clement chesnin wrote:
> > The error value is -1, not 0. That said, I wonder why you use TCP
> > instead of an intra-process mechanism.
> 
> This allowed use to clearly separate the audio and video part.

I'm afraid I fail to see the connection between the two. Could you
explain this a bit further?


> > + ? ? ? char *message = (char*)malloc(255*sizeof(char));
> >
> > sizeof(char) is fundamentally always one.
> > 'char message[255];' would be much simpler here.
> 
> Char array can not be dynamically modified

The size, no, but the contents, yes. You don't seem to be resizing so
there doesn't seem to be a point to go through the memory allocator. If
there is, please enlighten me.


Likewise, I don't really see a reason why the Bargraph struct needs to
be held as a pointer in the filter private struct, meaning that it could
be a member instead, reducing possible error conditions by one.

Some more points:

I'd like to highlight DrawpointsBlack and DrawpointsWhite; they might
be better readable as actual functions, marked inline if you'd like,
otherwise liberal application of `\' might improve readability. The rest
of the drawing code looks repetetive and could stand some factoring.

If you can eliminate sprintf in favour of snprintf, that'd be nice. Also
note that s(n)printf returns the number of bytes printed, meaning you
can avoid a strlen() call or two.

And a small point in closing: I see rather different indentations, some
evidently with tabs instead of spaces, or even tabs set to a width of 4.
Four space indentations, no tabs, throughout, would be nice.