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

[Roman Shaposhnik]

On Feb 12, 2008, at 6:13 PM, Michael Niedermayer wrote:
> On Fri, Jan 04, 2008 at 03:05:32PM +0800, Dan Maas wrote:
>>
>> Changes since my original 3-part post:
>> - macroblock lookup tables kept as described above
>> - tab and spacing issues fixed
>> - the few // comments switched to /* */
>>
>
>> You guys have better eyes for micro-optimizations than I do; if  
>> there are
>> more examples like Michael pointed out in the find_macroblock  
>> functions,
>> please let me know.
>
> Well its the patch submitters "job" to cleanup and optimize his  
> code if
> he wants to see it in svn. I surely will help but i wont approve  
> the patch
> as long as i find a single inefficient line in it. Maybe roman  
> will, maybe
> he will clean it up, if so fine, he is the dv maintainer and its his
> decission ...

    I'm finally back from a long series of business trips (hopefully  
I can stay put
for at least some time now) and I'll be slowly going through this patch
trying to clean it up where needed. Of course, Dan (or anybody for that
matter) is very welcome to help me along.

> The patch is big so
> if you wait for me to point to every one it will take a very long  
> time.
> Its your code and you know it better and could find and improve  
> suboptimal
> parts faster ...

   I do have a similar concern. Unlike DV25 I have no first hand  
experience
with DVCPRO HD. In fact I have to read the spec in order to familiarize
myself with it. That'll also take some time. Which means that Dan  
being active
in the polishing process can speed up things quite a lot.

>> and level*=iweight_table[pos] can be factored out of the if/else
>
> btw is the spec for this DV variant available freely somewhere?

    I can mail it to you privately.

>> +/* 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?

> 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?

>> +    /* 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 ?

Thanks,
Roman.