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

[Michael Niedermayer]

On Fri, Jan 04, 2008 at 03:05:32PM +0800, Dan Maas wrote:
> Thanks for your comments. After taking another look at the find_macroblock 
> functions, I've decided to withdraw that part of the patch. The lookup 
> tables are not as big as I thought (a few tens of KB) and it was not hard 
> to produce them for DV100. We can always switch to functions later if it 
> clearly improves performance.

Well you said it was faster to use functions (and obviously the functions
would be smaller and cleaner than the tables) so id like to see some
benchmarks between the table and function code before agreeing to adding
more tables.

Also note, to pass review the patch must be optimal and clean and not
contain anything which can be simplified. Also the patch must be free of
code duplications, similar function which could be factored into less code
and so on. An exception would be if the less simple code is faster of
course.

And because of the size of the patch this will take a few iterations. Ill
try to review it, and iam sure roman will as well, after all he is dv 
maintainer. Iam just trying to prevent some "not perfect" code from hitting
svn.


>
> (I kept the 16-bit lookup tables for DV25/50 and added 32-bit tables for 
> DV100. If this is too ugly we could make the DV25/50 tables into 32-bits or 
> find another approach, let me know.)
>
> The new DVCPRO HD patch is in two parts. The first patch (attached) makes 
> all of the code changes necessary to support DVCPRO HD, and the second 
> patch (URL below) adds the macroblock lookup tables.
> http://largefiles.maasdigital.com/largefiles/maas-dv100-2B-tables.diff.gz
>
> Changes since my original 3-part post:
> - macroblock lookup tables kept as described above
> - tab and spacing issues fixed

> - the few // comments switched to /* */

comments describing functions, structs, members of structs must be doxygen
compatible, the rest can use any syntax you like.
I think we had some examples of doxygen comments in the docs ...


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

> One more thing I wanted to ask about - I have a compile-time #define for 
> changing a quality-speed tradeoff in the DV100 encoder. It would be nicer 
> to do this as a run-time switch, but I'm not sure how best to get a 
> quality-speed parameter down at the codec level. Any help would be 
> appreciated.

What does the #define _exactly_ change? We have many variables in
AVCodecContext but its hard to suggest a specific one without knowing
precissely what that #define does.
Also maybe the algorithm could be changed to be high quality and fast at the
same time?


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

Democracy is the form of government in which you can choose your dictator
-------------- 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/20080104/8ec4245d/attachment.pgp>