[FFmpeg-devel] DVCPRO HD: request for review

Michael Niedermayer michaelni
Tue Aug 19 22:38:26 CEST 2008


On Tue, Aug 19, 2008 at 09:15:21AM -0700, Roman V. Shaposhnik wrote:
[...]
> > > @@ -1789,8 +1800,6 @@ static const uint16_t dv_place_422_525[2*10*27*5] = {
> > >   0x0b34, 0x2322, 0x2f46, 0x3b10, 0x1758,
> > >  };
> > >  
> > > -/* 2 channels per frame, 12 DIF sequences per channel,
> > > -   27 video segments per DIF sequence, 5 macroblocks per video segment */
> > >  static const uint16_t dv_place_422_625[2*12*27*5] = {
> > >   0x0c24, 0x2412, 0x3036, 0x0000, 0x1848,
> > >   0x0d24, 0x2512, 0x3136, 0x0100, 0x1948,
> > 
> > why is this comment removed?
> 
> purely cosmetics COSMETICS#3?

yes, and with an explanation why in the commit message :)


> 
> > > +    { .dsf = 1,
> > > +      .video_stype = 0x18,
> > > +      .frame_size = 288000,        /* SMPTE-370M - 720p50 100 Mbps */
> > > +      .difseg_size = 12,           /* also known as "DVCPRO HD" */
> > > +      .n_difchan = 2,
> > > +      .frame_rate = 50,
> > > +      .ltc_divisor = 50,
> > > +      .frame_rate_base = 1,
> > > +      .height = 720,
> > > +      .width = 960,
> > > +      .sar = {{1, 1}, {1, 1}},
> > > +      .video_place = dv_place_720p50,
> > > +      .pix_fmt = PIX_FMT_YUV422P,
> > > +      .bpm = 8,
> > > +      .block_sizes = block_sizes_dv100,
> > > +      .audio_stride = 90,
> > > +      .audio_min_samples = { 1580, 1452, 1053 }, /* for 48, 44.1 and 32Khz */
> > > +      .audio_samples_dist = { 1600, 1602, 1602, 1602, 1602 }, /* per SMPTE-314M */
> > > +      .audio_shuffle = dv_audio_shuffle525,
> > >      }
> > >  };
> > >  
> > 
> > ok, but seperate commit
> 
> yeah, but then the rest of the DVCPRO HD machinery should go there as
> well. ok?

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.


[...]
> 
> > > diff --git a/libavformat/isom.c b/libavformat/isom.c
> > > index c4a04b3..6f78581 100644
> > > --- a/libavformat/isom.c
> > > +++ b/libavformat/isom.c
> > > @@ -87,8 +87,11 @@ const AVCodecTag codec_movvideo_tags[] = {
> > >      { CODEC_ID_DVVIDEO, MKTAG('d', 'v', '5', 'p') }, /* DVCPRO50 PAL produced by FCP */
> > >      { CODEC_ID_DVVIDEO, MKTAG('d', 'v', '5', 'n') }, /* DVCPRO50 NTSC produced by FCP */
> > >      { CODEC_ID_DVVIDEO, MKTAG('A', 'V', 'd', 'v') }, /* AVID DV */
> > > -  //{ CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', '5') }, /* DVCPRO HD 50i produced by FCP */
> > > -  //{ CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', '6') }, /* DVCPRO HD 60i produced by FCP */
> > > +    { CODEC_ID_DVVIDEO, MKTAG('A', 'V', 'd', '1') }, /* AVID DV */
> > > +    { CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', 'q') }, /* DVCPRO HD 720p50 */
> > > +    { CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', 'p') }, /* DVCPRO HD 720p60 */
> > > +    { CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', '5') }, /* DVCPRO HD 50i produced by FCP */
> > > +    { CODEC_ID_DVVIDEO, MKTAG('d', 'v', 'h', '6') }, /* DVCPRO HD 60i produced by FCP */
> > >  
> > >      { CODEC_ID_VP3,     MKTAG('V', 'P', '3', '1') }, /* On2 VP3 */
> > >      { CODEC_ID_RPZA,    MKTAG('r', 'p', 'z', 'a') }, /* Apple Video (RPZA) */
> > 
> > ok but seperate commit
> 
> right. but when the codec is there.

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


[...]
> @@ -102,6 +107,17 @@ static void dv_build_unquantize_tables(DVVideoContext *s, uint8_t* perm)
>              }
>          }
>      }
> +
> +    for(a = 0; a < 4; a++) {
> +        for(q = 0; q < 16; q++) {
> +            for(i = 1; i < 64; i++) {
> +                s->dv100_idct_factor[0][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_y[i];
> +                s->dv100_idct_factor[1][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_c[i];
> +                s->dv100_idct_factor[2][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_y[i];
> +                s->dv100_idct_factor[3][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_c[i];
> +            }
> +        }
> +    }
>  }
>  
>  static av_cold int dvvideo_init(AVCodecContext *avctx)

ok


> @@ -353,10 +369,10 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>  {
>      int quant, dc, dct_mode, class1, j;
>      int mb_index, mb_x, mb_y, v, last_index;
> +    int y_stride, i;
>      DCTELEM *block, *block1;
>      int c_offset;
>      uint8_t *y_ptr;
> -    void (*idct_put)(uint8_t *dest, int line_size, DCTELEM *block);
>      const uint8_t *buf_ptr;
>      PutBitContext pb, vs_pb;
>      GetBitContext gb;
> @@ -365,6 +381,7 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>      DECLARE_ALIGNED_8(uint8_t, mb_bit_buffer[80 + 4]); /* allow some slack */
>      DECLARE_ALIGNED_8(uint8_t, vs_bit_buffer[5 * 80 + 4]); /* allow some slack */
>      const int log2_blocksize= 3-s->avctx->lowres;
> +    int dv100_dct_mode[5]; /* dct_mode is per-macroblock in DV100, not per-block as in DV25 and DV50 */
>  
>      assert((((int)mb_bit_buffer)&7)==0);
>      assert((((int)vs_bit_buffer)&7)==0);


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


> +            }
> +            mb->scan_table = s->dv_zigzag[mb->dct_mode];
> +
>              dc = dc << 2;
>              /* convert to unsigned because 128 is not added in the
>                 standard IDCT */

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


> +
> +        /* idct_put'ting luminance */
> +        if ((s->sys->pix_fmt == PIX_FMT_YUV420P) ||
> +            (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x >= (704 / 8)) ||
> +            (s->sys->height >= 720 && mb_y != 134)) {
> +            y_stride = (s->picture.linesize[0]<<((!dv100_dct_mode[mb_index])*log2_blocksize)) - (2<<log2_blocksize);
> +        } else {
> +            y_stride = 0;
> +        }
>          y_ptr = s->picture.data[0] + ((mb_y * s->picture.linesize[0] + mb_x)<<log2_blocksize);
> +        for(j = 0; j < 2; j++, y_ptr += y_stride) {

> +            for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))

i think there are too many statements on this line


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


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

[...]


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


[...]
> @@ -314,6 +315,13 @@ static const uint8_t dv_quant_shifts[22][4] = {
>  static const uint8_t dv_quant_offset[4] = { 6, 3, 0, 1 };
>  static const uint8_t dv_quant_areas[4] = { 6, 21, 43, 64 };
>  
> +/* quantization quanta by QNO for DV100 */
> +static const uint8_t dv100_qstep[16] = {
> +    1, /* QNO = 0 and 1 both have no quantization */
> +    1,
> +    2, 3, 4, 5, 6, 7, 8, 16, 18, 20, 22, 24, 28, 52
> +};
> +
>  /* NOTE: I prefer hardcoding the positioning of dv blocks, it is
>     simpler :-) */
>  

ok


[...]
>  static inline const DVprofile* dv_frame_profile(const uint8_t* frame)
>  {
> -    if ((frame[3] & 0x80) == 0) {      /* DSF flag */
> -        /* it's an NTSC format */
> -        if ((frame[80*5 + 48 + 3] & 0x4) && (frame[80*5 + 48] == dv_video_source)) { /* 4:2:2 sampling */
> -            return &dv_profiles[3]; /* NTSC 50Mbps */
> -        } else { /* 4:1:1 sampling */
> -            return &dv_profiles[0]; /* NTSC 25Mbps */
> -        }
> -    } else {
> -        /* it's a PAL format */
> -        if ((frame[80*5 + 48 + 3] & 0x4) && (frame[80*5 + 48] == dv_video_source)) { /* 4:2:2 sampling */
> -            return &dv_profiles[4]; /* PAL 50Mbps */
> -        } else if ((frame[5] & 0x07) == 0) { /* APT flag */
> -            return &dv_profiles[1]; /* PAL 25Mbps 4:2:0 */
> -        } else
> -            return &dv_profiles[2]; /* PAL 25Mbps 4:1:1 */
> -    }
> +   int i;
> +
> +   int dsf = (frame[3] & 0x80) >> 7;
> +
> +   int stype = frame[80*5 + 48 + 3] & 0x1f;
> +

> +   /* 576i50 25Mbps 4:1:1 is a special case */
> +   if (dsf == 1 && stype == 0 && frame[5] & 0x07) {
> +       return &dv_profiles[2];
> +   }

why is this special case needed ? cant it also be handled in the loop?


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


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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20080819/0a1ce8f1/attachment.pgp>



More information about the ffmpeg-devel mailing list