[FFmpeg-devel] DVCPRO HD: request for review
Roman V. Shaposhnik
rvs
Tue Aug 19 18:15:21 CEST 2008
On Tue, 2008-08-19 at 14:54 +0200, Michael Niedermayer wrote:
> > 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)
Well, yeah, but that'll be like the last commit. When everything is
done and the codec is there. I'll call it COSMETIC#1.
> > @@ -102,6 +107,18 @@ static void dv_build_unquantize_tables(DVVideoContext *s, uint8_t* perm)
> > }
> > }
> > }
> > +
> > +
>
> trailing whitespace
fixed.
> > - 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
committed to SVN as commit #1
> > @@ -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
committed to SVN as commit #2
> 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
commited to SVN as commit #3
> > @@ -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
this will be COSMETICS#2
> > @@ -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?
> > + { .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?
> > @@ -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
same here. this stuff is an integral part of the DVCPRO HD machinery.
> > 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.
> The remainder is not yet ok, i will review it in the next iteration of this
> patch.
The updated patch (against the SVN trunk) is attached for your further
review.
Thanks,
Roman.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dv.new.patch
Type: text/x-patch
Size: 181404 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080819/d6dd7723/attachment.bin>
More information about the ffmpeg-devel
mailing list