[FFmpeg-devel] DVCPRO HD: request for review

Roman V. Shaposhnik rvs
Thu Aug 21 10:01:01 CEST 2008


Michael Niedermayer ?????:

>i was trying to make the reviews easier for me by reducing the amount of
>code i have to look at. Iam rather short at time ATM due to SOC.
>  
>
That's totally understood. I can slice and dice this patch with Git any 
way you feel
it is convenievt for you to review. It was a commit order I was talking 
about.

>
>[...]
>  
>
>>>>diff --git a/libavformat/isom.c b/libavformat/isom.c
>>>>
>>>>        
>>>>
>well, it does no harm if its commited before
>Besides either these fourccs do correspond to DV or not its not as if
>that would change once libavcodec contains a decoder capable to decode them
>  
>
Sure. Will do.

>>@@ -390,11 +407,22 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>>             /* get the dc */
>>             dc = get_sbits(&gb, 9);
>>             dct_mode = get_bits1(&gb);
>>-            mb->scan_table = s->dv_zigzag[dct_mode];
>>             class1 = get_bits(&gb, 2);
>>+
>>+            if (DV_PROFILE_IS_HD(s->sys)) {
>>+                if (j == 0)
>>+                    dv100_dct_mode[mb_index] = dct_mode;
>>+                /* DV100 does not use the 2-4-8 DCT */
>>+                mb->dct_mode = 0;
>>+                mb->factor_table = s->dv100_idct_factor[((s->sys->height == 720)<<1)&(j < 4)][class1][quant];
>>+            } else {
>>+                dv100_dct_mode[mb_index] = 0;
>>-            mb->dct_mode = dct_mode;
>>+                mb->dct_mode = dct_mode;
>>-            mb->factor_table = s->dv_idct_factor[class1 == 3][dct_mode]
>>-                [quant + dv_quant_offset[class1]];
>>+                mb->factor_table = s->dv_idct_factor[class1 == 3][dct_mode]
>>+                    [quant + dv_quant_offset[class1]];
>>    
>>
>
>mix of functional and cosmetic changes
>  
>

Fair enough. I'll fix it.

>>@@ -470,61 +498,56 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>>         v = *mb_pos_ptr++;
>>         mb_x = v & 0xff;
>>         mb_y = v >> 8;
>>+        /* We work with 720p frames split in half. The odd half-frame (chan==2,3) is displaced :-( */
>>+        if (s->sys->height == 720 && ((s->buf[1]>>2)&0x3) == 0) {
>>+               mb_y = (mb_y + (36<<1)) % (45<<1);
>>+        }
>>    
>>
>
>performing modulo operations per block is not a good idea
>  
>

The compilers (even the dumb ones) usually take care of loop invariant 
pretty easily.
But since we're dealing with gcc here I can move it outside the loop myself.

>>+            for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))
>>    
>>
>
>i think there are too many statements on this line
>  
>
Well, that's the style I like very much. It lets me see clearly what the 
loop is iterating over instead
of looking all over the body. Again, this is my personal choice and it 
DOES make my life
easier. Now, since there's usually two people in the world who look at 
dv*.[ch] files in FFmpeg
close enough to be annoyed by a choice of a style -- that would be you 
and me. And given that
I spend much more time with the code than you do, can I, please, have it 
the way it makes
my life easier?

>>+                 if (s->sys->pix_fmt == PIX_FMT_YUV422P && s->sys->width == 720 && i)
>>+                     y_ptr -= (1<<log2_blocksize);
>>+                 else
>>+                     s->idct_put[mb->dct_mode && log2_blocksize==3](y_ptr,
>>+                                                                    s->picture.linesize[0]<<dv100_dct_mode[mb_index],
>>+                                                                    block);
>>    
>>
>
>mb->dct_mode and dv100_dct_mode[] are badly choosen names
>they arent the same yet have so similar names
>and what is dct_mode = 0 or =1 this doesnt say anything
>both should be renamed to names that make sense, like is_248dct
>
>also it seems the local dv100_dct_mode[] and in mb struct dct_mode is
>inconsistant design.
>  
>
Good point. I'll take care of that. Thanks for picking on this.

>>+        }
>>+
>>+        /* idct_put'ting chrominance */
>>         c_offset = (((mb_y>>(s->sys->pix_fmt == PIX_FMT_YUV420P)) * s->picture.linesize[1] +
>>                      (mb_x>>((s->sys->pix_fmt == PIX_FMT_YUV411P)?2:1)))<<log2_blocksize);
>>-
>>-        for(j = 0;j < 6; j++) {
>>-            idct_put = s->idct_put[mb->dct_mode && log2_blocksize==3];
>>-            if (s->sys->pix_fmt == PIX_FMT_YUV422P) { /* 4:2:2 */
>>-                if (j == 0 || j == 2) {
>>-                    /* Y0 Y1 */
>>-                    idct_put(y_ptr + ((j >> 1)<<log2_blocksize),
>>-                             s->picture.linesize[0], block);
>>-                } else if(j > 3) {
>>-                    /* Cr Cb */
>>-                    idct_put(s->picture.data[6 - j] + c_offset,
>>-                             s->picture.linesize[6 - j], block);
>>-                }
>>-                /* note: j=1 and j=3 are "dummy" blocks in 4:2:2 */
>>-            } else { /* 4:1:1 or 4:2:0 */
>>-                if (j < 4) {
>>-                    if (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x < (704 / 8)) {
>>-                        /* NOTE: at end of line, the macroblock is handled as 420 */
>>-                        idct_put(y_ptr + (j<<log2_blocksize), s->picture.linesize[0], block);
>>-                    } else {
>>-                        idct_put(y_ptr + (((j & 1) + (j >> 1) * s->picture.linesize[0])<<log2_blocksize),
>>-                                 s->picture.linesize[0], block);
>>-                    }
>>-                } else {
>>-                    if (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x >= (704 / 8)) {
>>-                        uint64_t aligned_pixels[64/8];
>>-                        uint8_t *pixels= (uint8_t*)aligned_pixels;
>>-                        uint8_t *c_ptr, *c_ptr1, *ptr, *ptr1;
>>-                        int x, y, linesize;
>>-                        /* NOTE: at end of line, the macroblock is handled as 420 */
>>-                        idct_put(pixels, 8, block);
>>-                        linesize = s->picture.linesize[6 - j];
>>-                        c_ptr = s->picture.data[6 - j] + c_offset;
>>-                        ptr = pixels;
>>-                        for(y = 0;y < (1<<log2_blocksize); y++) {
>>-                            ptr1= ptr + (1<<(log2_blocksize-1));
>>-                            c_ptr1 = c_ptr + (linesize<<log2_blocksize);
>>-                            for(x=0; x < (1<<(log2_blocksize-1)); x++){
>>-                                c_ptr[x]= ptr[x]; c_ptr1[x]= ptr1[x];
>>-                            }
>>-                            c_ptr += linesize;
>>-                            ptr += 8;
>>-                        }
>>-                    } else {
>>-                        /* don't ask me why they inverted Cb and Cr ! */
>>-                        idct_put(s->picture.data[6 - j] + c_offset,
>>-                                 s->picture.linesize[6 - j], block);
>>-                    }
>>-                }
>>+        for(j=2; j; j--) {
>>+            uint8_t *c_ptr = s->picture.data[j] + c_offset;
>>+            if (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x >= (704 / 8)) {
>>+                  uint64_t aligned_pixels[64/8];
>>+                  uint8_t *pixels = (uint8_t*)aligned_pixels;
>>+                  uint8_t *c_ptr1, *ptr1;
>>+                  int x, y;
>>+                  s->idct_put[mb->dct_mode && log2_blocksize==3](pixels, 8, block);
>>+                  for(y = 0; y < (1<<log2_blocksize); y++, c_ptr += s->picture.linesize[j], pixels += 8) {
>>+                      ptr1= pixels + (1<<(log2_blocksize-1));
>>+                      c_ptr1 = c_ptr + (s->picture.linesize[j]<<log2_blocksize);
>>+                      for(x=0; x < (1<<(log2_blocksize-1)); x++)
>>+                          c_ptr[x]= pixels[x]; c_ptr1[x]= ptr1[x];
>>+                  }
>>+                  block += 64; mb++;
>>+            } else {
>>    
>>
>
>  
>
>>+                  y_stride = (mb_y == 134) ? (1<<log2_blocksize) :
>>+                                             s->picture.linesize[j]<<((!dv100_dct_mode[mb_index])*log2_blocksize);
>>+                  for (i=0; i<(1<<(s->sys->bpm==8)); i++, block += 64, mb++, c_ptr += y_stride)
>>    
>>
>
>this as well has far too much on one line to be readable
>  
>

See aove.

>>@@ -971,6 +994,14 @@ static int dv_decode_mt(AVCodecContext *avctx, void* sl)
>>     /* byte offset of this channel's data */
>>     int chan_offset = chan * s->sys->difseg_size * 150 * 80;
>> 
>>+    /* DIF sequence */
>>+    int seq = chan_slice / 27;
>>    
>>
>                 ^^^^^^^^^^^^^^^
>  
>
>>+
>>+    /* in 1080i50 and 720p50 some seq are unused */
>>+    if ((DV_PROFILE_IS_1080i50(s->sys) && chan != 0 && seq == 11) ||
>>+        (DV_PROFILE_IS_720p50(s->sys) && seq > 9))
>>+        return 0;
>>+
>>     dv_decode_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]);
>>     return 0;
>>    
>>
>
>there is something that can be facotored out in a seperate commit
>  
>

Sure.

>>+
>>+   for (i=0; i<sizeof(dv_profiles)/sizeof(DVprofile); i++)
>>+       if (dsf == dv_profiles[i].dsf && stype == dv_profiles[i].video_stype)
>>+           return &dv_profiles[i];
>>+
>>+   return NULL;
>> }
>> 
>>    
>>
>
>  
>
>> static inline const DVprofile* dv_codec_profile(AVCodecContext* codec)
>> {
>>     int i;
>> 
>>-    if (codec->width != 720)
>>-        return NULL;
>>-
>>     for (i=0; i<sizeof(dv_profiles)/sizeof(DVprofile); i++)
>>-       if (codec->height == dv_profiles[i].height && codec->pix_fmt == dv_profiles[i].pix_fmt)
>>-           return &dv_profiles[i];
>>+       if (codec->height == dv_profiles[i].height && codec->pix_fmt == dv_profiles[i].pix_fmt &&
>>+           codec->width == dv_profiles[i].width)
>>+               return &dv_profiles[i];
>> 
>>     return NULL;
>> }
>>    
>>
>
>ok
>
>
>[...]
>  
>
>>@@ -192,11 +197,17 @@ static int dv_extract_audio_info(DVDemuxContext* c, uint8_t* frame)
>> 
>>     smpls = as_pack[1] & 0x3f; /* samples in this frame - min. samples */
>>     freq = (as_pack[4] >> 3) & 0x07; /* 0 - 48KHz, 1 - 44,1kHz, 2 - 32 kHz */
>>-    stype = (as_pack[3] & 0x1f); /* 0 - 2CH, 2 - 4CH */
>>+    stype = (as_pack[3] & 0x1f); /* 0 - 2CH, 2 - 4CH, 3 - 8CH */
>>     quant = as_pack[4] & 0x07; /* 0 - 16bit linear, 1 - 12bit nonlinear */
>> 
>>    
>>
>
>ok and seperate cosmetic commit
>
>
>  
>
>>     /* note: ach counts PAIRS of channels (i.e. stereo channels) */
>>-    ach = (stype == 2 || (quant && (freq == 2))) ? 2 : 1;
>>+    if (stype == 3) {
>>+        ach = 4;
>>+    } else if (stype == 2 || (quant && (freq == 2))) {
>>+        ach = 2;
>>+    } else {
>>+        ach = 1;
>>+    }
>> 
>>     /* Dynamic handling of the audio streams in DV */
>>     for (i=0; i<ach; i++) {
>>    
>>
>
>this is a mix of cosmetic and functional changes
>the functional change really would be
>
>+if (stype == 3) {
>+    ach = 4;
>+} else
> ach = (stype == 2 || (quant && (freq == 2))) ? 2 : 1;
>
>
>  
>
>>@@ -276,7 +287,7 @@ DVDemuxContext* dv_init_demux(AVFormatContext *s)
>> 
>>     c->sys = NULL;
>>     c->fctx = s;
>>-    c->ast[0] = c->ast[1] = NULL;
>>+    memset(c->ast, 0, sizeof(c->ast));
>>     c->ach = 0;
>>     c->frames = 0;
>>     c->abytes = 0;
>>    
>>
>
>ok and seperate commit
>
>
>[...]
>  
>
>------------------------------------------------------------------------
>
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel at mplayerhq.hu
>https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>  
>





More information about the ffmpeg-devel mailing list