[FFmpeg-devel] [PATCH] DVCPRO HD - revised two-part patch

[Michael Niedermayer]

On Tue, Feb 12, 2008 at 06:27:18PM -0800, Roman Shaposhnik wrote:
[...]
> >> +/* interleave scanlines from even- and odd-numbered blocks */
> >> +static void dv100_interleave(DVVideoContext *s, int mb_x, int  
> >> mb_y, uint8_t *y_ptr, int c_offset)
> >
> > doxygen and the description is not sufficient to understand what the
> > function does.
> 
>    Surely you don't mean that every internal function should be  
> documented, do you?

Well, no not every. But it can help alot if someone tries to understand
some code. Thats not only for reviewing but also for anyone later who happens
to look at the code.
a sqrt() is self explanatory() a decode_block() as well.
But when i looked at dv100_interleave() the first time i wondered what blocks
from where get their lines interleaved to where.

If that function would have taken 2 uint8_t *src and 2 *dst then again it
would have been all clear (with a note about the blocks being 8x8 pixels).
But as is i think it could be improved. I dont say it needs 10 lines, but
even 2 more lines of explanation would help it IMHO.


> 
> > This as well as the similar existing code is a mess, it would be  
> > nice if that
> > would be cleaned up eventually before too many copy and pasted  
> > variants of
> > it accumulate. That doesnt mean iam not ok with this as it is jst that
> > iam pretty sure the whole can be done alot cleaner than having a  
> > large special
> > case for every variant in the loop.
> 
>    Any suggestions on how to fix it? At least the current case?

Ill need to look at it a little longer to awnser that ...



> 
> >> +    /* DSF flag */
> >> +    int dsf = (frame[3] & 0x80) >> 7;
> >> +
> >> +    /* VAUX stype */
> >> +    int stype = frame[80*5 + 48 + 3] & 0x1f;
> >
> > Please give variables sane names instead if putting the sane name  
> > before
> > each in a comment.
> 
>     you mean dsf_flag & vaux_stype ?

yes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080213/2ebd2f09/attachment.pgp>