[FFmpeg-devel] DVCPRO HD: request for review

[Michael Niedermayer]

On Wed, Aug 27, 2008 at 01:34:01PM -0700, Roman Shaposhnik wrote:
> This is an updated revision of the patch. I tried to take care of
> all the comments that Michael made, except for this one:
> 
> > > +   /* 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?
> 
> The special case is there to stay :-( The trouble is that I don't
> have a spec for the twist of DV this is working around (IEC 61834)
> and I can't see a reliable method for moving it inside the loop.

[...]

> @@ -59,8 +64,8 @@ typedef struct DVVideoContext {
>  
>  /* MultiThreading - dv_anchor applies to entire DV codec, not just the avcontext */
>  /* one element is needed for each video segment in a DV frame */
> -/* at most there are 2 DIF channels * 12 DIF sequences * 27 video segments (PAL 50Mbps) */
> -#define DV_ANCHOR_SIZE (2*12*27)
> +/* at most there are 4 DIF channels * 12 DIF sequences * 27 video segments (1080i50) */
> +#define DV_ANCHOR_SIZE (4*12*27)
>  
>  static void* dv_anchor[DV_ANCHOR_SIZE];
>  

ok


[...]
> @@ -349,6 +366,7 @@ 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;
> @@ -360,6 +378,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 is_field_mode[5];
>  
>      assert((((int)mb_bit_buffer)&7)==0);
>      assert((((int)vs_bit_buffer)&7)==0);
> @@ -386,10 +405,18 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>              dc = get_sbits(&gb, 9);
>              dct_mode = get_bits1(&gb);
>              class1 = get_bits(&gb, 2);
> +            if (DV_PROFILE_IS_HD(s->sys)) {
> +                mb->idct_put = s->idct_put[0];
> +                mb->scan_table = s->dv_zigzag[0];
> +                mb->factor_table = s->dv100_idct_factor[((s->sys->height == 720)<<1)&(j < 4)][class1][quant];
> +                is_field_mode[mb_index] = !j && dct_mode;
> +            } else {

the dct_mode bit does nothing and can have any value for j>0 ?


[...]
>  #define DV_PROFILE_BYTES (6*80) /* 6 DIF blocks */
>  
> -/* largest possible DV frame, in bytes (PAL 50Mbps) */
> -#define DV_MAX_FRAME_SIZE 288000
> +/* largest possible DV frame, in bytes (1080i50) */
> +#define DV_MAX_FRAME_SIZE 576000
>  
>  /* maximum number of blocks per macroblock in any DV format */
>  #define DV_MAX_BPM 8

ok


[...]
>  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)
> +       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

[...]
> @@ -196,6 +201,9 @@ static int dv_extract_audio_info(DVDemuxContext* c, uint8_t* frame)
>      quant = as_pack[4] & 0x07; /* 0 - 16bit linear, 1 - 12bit nonlinear */
>  
>      /* note: ach counts PAIRS of channels (i.e. stereo channels) */
> +    if (stype == 3) {
> +        ach = 4;
> +    } else 
>      ach = (stype == 2 || (quant && (freq == 2))) ? 2 : 1;
>  
>      /* Dynamic handling of the audio streams in DV */

ok


[...]
> @@ -391,6 +410,11 @@ static int dv_read_header(AVFormatContext *s,
>          return AVERROR(EIO);
>  
>      c->dv_demux->sys = dv_frame_profile(c->buf);
> +    if (!c->dv_demux->sys) {
> +        av_log(s, AV_LOG_ERROR, "Can't determine profile of DV input stream.\n");
> +        return -1;
> +    }
> +
>      s->bit_rate = av_rescale(c->dv_demux->sys->frame_size * 8,
>                               c->dv_demux->sys->frame_rate,
>                               c->dv_demux->sys->frame_rate_base);

ok

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080828/d854aaa1/attachment.pgp>