[FFmpeg-devel] [PATCH] DVCPRO HD [1/3] dynamic DV macroblock lookup
Michael Niedermayer
michaelni
Thu Jan 3 20:09:18 CET 2008
On Thu, Jan 03, 2008 at 09:07:46PM +0800, Dan Maas wrote:
> This is a series of 3 incremental patches that add DVCPRO HD support to
> ffmpeg.
>
> Patch 1 of 3 removes the look-up tables used by the DV codec to determine
> macroblock locations, and replaces them with functions that computes the
> macroblock locations at runtime. I feel this is a necessary step before
> adding DVCPRO HD support, because the look-up tables that would be required
> for the HD format would be very large. DVCPRO HD has about four times as
> many macroblocks as NTSC/PAL formats, and the coordinate numbers won't fit
> in 16-bit entries anymore, so the table would have to grow even more than
> that.
>
> I cannot detect any measurable speed change with this patch compared to the
> current look-up table code. If anything it may be slightly faster due to
> smaller cache footprint.
>
> (apologies for compressing the patch; it's 100KB uncompressed due to the
> removal of large look-up tables)
[...]
> @@ -483,9 +483,8 @@
> block = &sblock[0][0];
> mb = mb_data;
> for(mb_index = 0; mb_index < 5; mb_index++) {
> - v = *mb_pos_ptr++;
> - mb_x = v & 0xff;
> - mb_y = v >> 8;
> + mb_x = *mb_pos_ptr++;
> + mb_y = *mb_pos_ptr++;
tabs are forbidden in svn
[...]
> }
>
> +/* macroblock permutations common to all formats */
> +static const uint8_t super_x_order[] = {2,1,3,0,4};
> +static const uint8_t super_y_bases[] = {2,6,8,0,4};
> +
comment is not doxygen compatible
> +void find_macroblock_dv25_411(const struct DVprofile *sys, int channel, int seq, int av, int seg, int mbloc[10])
needs ff_ prefix or static
> +{
> + /* starting x coordinate of horizontal superblocks, in units of blocks */
> + static const uint8_t super_x_starts[] = {0,4,9,13,18};
> +
> + int mb_index;
> + int *mb = &mbloc[0];
> + for (mb_index = 0; mb_index < 5; mb_index++, mb += 2) {
> + /* index of this block within the DIF sequence (0-135) */
> + int vid_block_num = av*15 + seg*5 + mb_index;
> +
> + /* X and Y indices of the superblock we're inside */
> + int super_x = super_x_order[vid_block_num % 5];
> + int super_y = (super_y_bases[vid_block_num % 5] + seq) % sys->difseg_size;
> +
> + /* index within the superblock */
> + int within_super = vid_block_num / 5;
% and / are very slow, please use your brain! (i guess gcc was smarter and
optimized it away)
hint:
int super_x = super_x_order[mb_index];
int super_y = (super_y_bases[mb_index] + seq) % sys->difseg_size;
int within_super = av*3 + seg;
> +
> + /* locate us within the superblock's "Tetris" pattern */
> + int within_super_x, within_super_y;
> +
> + if (super_x & 1) {
> + /* super_x is ODD */
> + if (within_super < 3) {
> + within_super_x = 0;
> + within_super_y = 3 + within_super;
> + } else if (within_super < 9) {
> + within_super_x = 1;
> + within_super_y = 5 - (within_super-3);
> + } else if (within_super < 15) {
> + within_super_x = 2;
> + within_super_y = (within_super-9);
> + } else if (within_super < 21) {
> + within_super_x = 3;
> + within_super_y = 5 - (within_super-15);
> + } else {
> + within_super_x = 4;
> + within_super_y = (within_super-21);
> + }
within_super_x= (within_super+3)/6;
within_super_y= (within_super+3)%6;
if(within_super_x&1)
within_super_y= 5 - within_super_y;
> + } else {
> + /* super_x is EVEN */
> + if (within_super < 6) {
> + within_super_x = 0;
> + within_super_y = within_super;
> + } else if (within_super < 12) {
> + within_super_x = 1;
> + within_super_y = 5 - (within_super-6);
> + } else if (within_super < 18) {
> + within_super_x = 2;
> + within_super_y = (within_super - 12);
> + } else if (within_super < 24) {
> + within_super_x = 3;
> + within_super_y = 5 - (within_super-18);
> + } else {
> + within_super_x = 4;
> + within_super_y = (within_super-24);
within_super_x= (within_super)/6;
within_super_y= (within_super)%6;
if(within_super_x&1)
within_super_y= 5 - within_super_y;
> + /* special case for rightmost superblock */
> + if (super_x == 4) {
> + within_super_y *= 2;
> + }
> + }
indention is messed up and the check is unneeded in your current code
> + }
> +
> + mb[0] = 4*(super_x_starts[super_x] + within_super_x);
> + mb[1] = 6*super_y + within_super_y;
> + }
> +}
> +
> +/* return x,y given an index into the 9x3 brick pattern */
> +static void macroblock_brick_pattern(int within_super, int *within_super_x, int *within_super_y)
> +{
> + if (within_super < 3) {
> + *within_super_x = 0;
> + *within_super_y = within_super;
> + } else if (within_super < 6) {
> + *within_super_x = 1;
> + *within_super_y = 2 - (within_super-3);
> + } else if (within_super < 9) {
> + *within_super_x = 2;
> + *within_super_y = (within_super-6);
> + } else if (within_super < 12) {
> + *within_super_x = 3;
> + *within_super_y = 2 - (within_super-9);
> + } else if (within_super < 15) {
> + *within_super_x = 4;
> + *within_super_y = (within_super-12);
> + } else if (within_super < 18) {
> + *within_super_x = 5;
> + *within_super_y = 2 - (within_super-15);
> + } else if (within_super < 21) {
> + *within_super_x = 6;
> + *within_super_y = (within_super-18);
> + } else if (within_super < 24) {
> + *within_super_x = 7;
> + *within_super_y = 2 - (within_super-21);
> + } else {
> + *within_super_x = 8;
> + *within_super_y = (within_super-24);
> + }
> +}
this can be simplified similarely and then factored
> +
> +void find_macroblock_dv25_420(const struct DVprofile *sys, int channel, int seq, int av, int seg, int mbloc[10])
> +{
> + int mb_index;
> + int *mb = &mbloc[0];
> + for (mb_index = 0; mb_index < 5; mb_index++, mb += 2) {
> + /* index of this block within the DIF sequence (0-135) */
> + int vid_block_num = av*15 + seg*5 + mb_index;
> +
> + /* X and Y indices of the superblock we're inside */
> + int super_x = super_x_order[vid_block_num % 5];
> + int super_y = (super_y_bases[vid_block_num % 5] + seq) % sys->difseg_size;
> +
> + /* index within the superblock */
> + int within_super = vid_block_num / 5;
> +
> + /* locate us within the superblock's "brick" pattern */
> + int within_super_x, within_super_y;
> +
> + macroblock_brick_pattern(within_super, &within_super_x, &within_super_y);
> +
> + mb[0] = 2*(9*super_x + within_super_x);
> + mb[1] = 2*(3*super_y + within_super_y);
> + }
> +}
This is duplicated, please clean and simplify your code before submitting!!!
Also note to roman, please dont apply this before it has been simplified and
cleaned up, the author should do this before it reaches svn!
very similar issues exist in the remainder of the code
Also please split the table removial out of the addition+changes
so you dont need to compress the patch (the actual table removial can be
compressed but thats likely going to be approved quickly while the rest looks
like it will need a few iterations)
[...]
[...]
--
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/20080103/377c93d5/attachment.pgp>
More information about the ffmpeg-devel
mailing list