[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