[FFmpeg-devel] DVCPRO HD: request for review

Michael Niedermayer michaelni
Tue Aug 19 14:54:25 CEST 2008


On Sun, Aug 17, 2008 at 11:06:39AM -0700, Roman V. Shaposhnik wrote:
> I would like to start integrating the support for DVCPRO HD and
> the first patch is for the decoder. There's a couple of things
> I would like to note about it:
>    0. It turned out to be quite different compared to what Dan
>       has originally submitted.
>    1. There's a trick I play with 720p formats in order to workaround
>       the braindead spec that says -- a single DV frame has two
>       complete progressive 720p frames. The trick is based on the
>       observation I made: last 2 DV channels (2,3) can be treated as
>       a separate DV frame if you're willing to compensate for a vertical
>       displacement of odd ones. Thus the trick is 100% safe with
>       raw DV and it seems that most DV authoring tools are writing these
>       half-frames when the output is something like .mov anyway.
>    2. The new set of tables is added. I would like to proceed with
>       adding them first and them trying to figure out whether an
>       alternative technique (such as generating tables on the fly, etc.)
>       is feasible.
> 
> That's pretty much it. Please review and let me know what should be 
> polished before I commit it.
> 
> Thanks,
> Roman. 

> diff --git a/libavcodec/dv.c b/libavcodec/dv.c
> index 30e0b1d..82ded17 100644
> --- a/libavcodec/dv.c
> +++ b/libavcodec/dv.c
> @@ -9,6 +9,10 @@
>   * 50 Mbps (DVCPRO50) support
>   * Copyright (c) 2006 Daniel Maas <dmaas at maasdigital.com>
>   *
> + * 100 Mbps (DVCPRO HD) support
> + * Initial code by Daniel Maas <dmaas at maasdigital.com> (funded by BBC R&D)
> + * Final code by Roman Shaposhnik
> + *
>   * Many thanks to Dan Dennedy <dan at dennedy.org> for providing wealth
>   * of DV technical info.
>   *

ok (= this part up to the previous empty line can be commited)


[...]

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


> @@ -102,6 +107,18 @@ static void dv_build_unquantize_tables(DVVideoContext *s, uint8_t* perm)
>              }
>          }
>      }
> + 
> +    

trailing whitespace


[...]
> -    BlockInfo mb_data[5 * 6], *mb, *mb1;
> -    DECLARE_ALIGNED_16(DCTELEM, sblock[5*6][64]);
> +    BlockInfo mb_data[5 * DV_MAX_BPM], *mb, *mb1;
> +    DECLARE_ALIGNED_16(DCTELEM, sblock[5*DV_MAX_BPM][64]);

ok but all the DV_MAX_BPM additions should be in a sepeare commit


[...]

> @@ -376,25 +390,36 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
>      block1 = &sblock[0][0];
>      mb1 = mb_data;
>      init_put_bits(&vs_pb, vs_bit_buffer, 5 * 80);
> -    for(mb_index = 0; mb_index < 5; mb_index++, mb1 += 6, block1 += 6 * 64) {
> +    for(mb_index = 0; mb_index < 5; mb_index++, mb1 += s->sys->bpm, block1 += s->sys->bpm * 64) {

ok, but all the additions of bps should as well be in a seperate commit


>          /* skip header */
>          quant = buf_ptr[3] & 0x0f;
>          buf_ptr += 4;
>          init_put_bits(&pb, mb_bit_buffer, 80);
>          mb = mb1;
>          block = block1;
> -        for(j = 0;j < 6; j++) {
> -            last_index = block_sizes[j];
> +        for(j = 0;j < s->sys->bpm; j++) {
> +            last_index = s->sys->block_sizes[j];

the addition of block_sizes into the struct as well as its use and the
removal of the block_sizes array are a seperate change and should also be
a sperate commit


[...]
> @@ -999,7 +1027,7 @@ static int dv_encode_mt(AVCodecContext *avctx, void* sl)
>  
>  #ifdef CONFIG_DECODERS
>  /* NOTE: exactly one frame must be given (120000 bytes for NTSC,
> -   144000 bytes for PAL - or twice those for 50Mbps) */
> +   144000 bytes for PAL - or twice those for 50Mbps - or 4x for 100Mbps) */
>  static int dvvideo_decode_frame(AVCodecContext *avctx,
>                                   void *data, int *data_size,
>                                   const uint8_t *buf, int buf_size)

ok but only together with other pure cosmetic changes


> diff --git a/libavcodec/dvdata.h b/libavcodec/dvdata.h
> index c725e1a..f9f6eff 100644
> --- a/libavcodec/dvdata.h
> +++ b/libavcodec/dvdata.h
> @@ -38,6 +38,7 @@
>   */
>  typedef struct DVprofile {
>      int              dsf;                 /* value of the dsf in the DV header */
> +    int              video_stype;         /* stype for VAUX source pack */
>      int              frame_size;          /* total size of one frame in bytes */
>      int              difseg_size;         /* number of DIF segments per DIF channel */
>      int              n_difchan;           /* number of DIF channels per frame */

ok


[...]
> @@ -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?


[...]

> @@ -2521,8 +6149,18 @@ static const av_unused int dv_audio_frequency[3] = {
>      48000, 44100, 32000,
>  };
>  
> +/* macroblock bit budgets */
> +static const uint8_t block_sizes_dv2550[8] = {
> +    112, 112, 112, 112, 80, 80, 0, 0,
> +};
> +
> +static const uint8_t block_sizes_dv100[8] = {
> +    80, 80, 80, 80, 80, 80, 64, 64,
> +};
> +

ok but seperate commit or together with the other block_size changes


[...]

> +    },
> +    { .dsf = 0,
> +      .video_stype = 0x14,
> +      .frame_size = 480000,        /* SMPTE-370M - 1080i60 100 Mbps */
> +      .difseg_size = 10,           /* also known as "DVCPRO HD" */
> +      .n_difchan = 4,
> +      .frame_rate = 30000,
> +      .ltc_divisor = 30,
> +      .frame_rate_base = 1001,
> +      .height = 1080,
> +      .width = 1280,
> +      .sar = {{1, 1}, {1, 1}},
> +      .video_place = dv_place_1080i60,
> +      .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,
> +    },
> +    { .dsf = 1,
> +      .video_stype = 0x14,
> +      .frame_size = 576000,        /* SMPTE-370M - 1080i50 100 Mbps */
> +      .difseg_size = 12,           /* also known as "DVCPRO HD" */
> +      .n_difchan = 4,
> +      .frame_rate = 25,
> +      .frame_rate_base = 1,
> +      .ltc_divisor = 25,
> +      .height = 1080,
> +      .width = 1440,
> +      .sar = {{1, 1}, {1, 1}},
> +      .video_place = dv_place_1080i50,
> +      .pix_fmt = PIX_FMT_YUV422P,
> +      .bpm = 8,
> +      .block_sizes = block_sizes_dv100,
> +      .audio_stride = 108,
> +      .audio_min_samples = { 1896, 1742, 1264 }, /* for 48, 44.1 and 32Khz */
> +      .audio_samples_dist = { 1920, 1920, 1920, 1920, 1920 },
> +      .audio_shuffle = dv_audio_shuffle625,
> +    },
> +    { .dsf = 0,
> +      .video_stype = 0x18,
> +      .frame_size = 240000,        /* SMPTE-370M - 720p60 100 Mbps */
> +      .difseg_size = 10,           /* also known as "DVCPRO HD" */
> +      .n_difchan = 2,
> +      .frame_rate = 60000,
> +      .ltc_divisor = 60,
> +      .frame_rate_base = 1001,
> +      .height = 720,
> +      .width = 960,
> +      .sar = {{1, 1}, {1, 1}},
> +      .video_place = dv_place_720p60,
> +      .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,
> +    },
> +    { .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


> @@ -2632,42 +6364,47 @@ enum dv_pack_type {
>       dv_unknown_pack  = 0xff,
>  };
>  
> +#define DV_PROFILE_IS_HD(p) ((p)->video_stype & 0x10)
> +#define DV_PROFILE_IS_1080i50(p) (((p)->video_stype == 0x14) && ((p)->dsf == 1))
> +#define DV_PROFILE_IS_720p50(p)  (((p)->video_stype == 0x18) && ((p)->dsf == 1))
> +
>  /* minimum number of bytes to read from a DV stream in order to determine the profile */
>  #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

ok


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

The remainder is not yet ok, i will review it in the next iteration of this
patch.

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/3d1da9d6/attachment.pgp>



More information about the ffmpeg-devel mailing list