[FFmpeg-devel] [PATCH] DVCPRO HD [1/3] dynamic DV macroblock lookup

[Roman Shaposhnik]

Hi Dan!

Long time no see ;-)

On Thu, 2008-01-03 at 21:07 +0800, Dan Maas wrote:
> This is a series of 3 incremental patches that add DVCPRO HD support to 
> ffmpeg.

  This is wonderful news!

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

  Really? Have you benchmarked it on the existing DV material or only
on DVCPRO HD? Also, what architectures did you run your tests on? 
I'm a bit surprised when you say it could even be faster, since for the
original DV the lookup tables seemed to give us some advantage over the
computation of offsets.

  Now, for some high-level comments (once we sort these issues out, I'll
do a more thorough review):


diff -ur ffmpeg-dv100-0/libavcodec/dv.c ffmpeg-dv100-A/libavcodec/dv.c
--- ffmpeg-dv100-0/libavcodec/dv.c      2008-01-04 03:46:55.000000000 +0800
+++ ffmpeg-dv100-A/libavcodec/dv.c      2008-01-03 23:02:06.000000000
+0800
@@ -364,10 +364,10 @@
 /* mb_x and mb_y are in units of 8 pixels */
 static inline void dv_decode_video_segment(DVVideoContext *s,
                                            uint8_t *buf_ptr1,
-                                           const uint16_t *mb_pos_ptr)
+                                           const int mb_pos_ptr[10])

  Somehow I always feel better when I know the size/signess explicitly. 


........................

@@ -1020,8 +1191,22 @@
     /* byte offset of this channel's data */
     int chan_offset = chan * s->sys->difseg_size * 150 * 80;

+    /* macroblock locations (x,y for each of the 5 macroblocks) */
+    int mbloc[5*2];
+
+    /* DIF sequence */
+    int seq = chan_slice / 27;
+
+    /* AV sequence */
+    int av = (chan_slice/3) % 9;
+
+    /* video segment */
+    int seg = chan_slice % 3;
+
+    s->sys->find_macroblock(s->sys, chan, seq, av, seg, mbloc);
+
     dv_encode_video_segment(s, &s->buf[((chan_slice/27)*6+(chan_slice/3)+chan_slice*5+7)*80 + chan_offset],
-                            &s->sys->video_place[slice*5]);
+                            mbloc);

  Hm. Have you though about precomputing the entire table at once instead of this on-demand
per-slice computations? 

Thanks,
Roman.