[FFmpeg-devel] [FFmpeg-cvslog] r15010 - in trunk: Changelog libavcodec/dv.c libavcodec/dvdata.h libavformat/dv.c libavformat/isom.c

[Michael Niedermayer]

On Sat, Sep 06, 2008 at 04:14:23PM -0700, Roman Shaposhnik wrote:
> [ I believe that -cvslog should ALWAYS be read-only, hence reposting here]
> 
> First of all, thanks for the review. Some of the comments are quite good
> and I have no problem with fixing the issues they've identified. However,
> I do have a couple of questions. See inlined:
> 

> > >  /* 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)
> >
> > comment is not doxygen compatible
> 
> I have no problem making it compatible, but what's the point documenting
> a private macro? It is not like we are talking about a public header file
> here, where all your comments on non-doxygen comments will be promptly dealt with.

The point is that if one is browsing the doxygen doc be that local or one of
the online ones the field would be documented.
Not everyone reads the doxy because he wants to use libav* in his own program
some might very well want to work on libav* itself.


> 
> > > +
> > > +    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];
> > > +            }
> > > +        }
> > > +    }
> > >  }
> > >  
> >
> > as video is eiher 1080 or 720, it should not be needed to store both.
> 
> You've ok'ed this very hunk here:
>     http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-August/051782.html
> So does it mean that you can withdraw other OKed hunks? ;-)
> 
> Seriously, though -- I would like to NOT constraint the codec by the
> initial stream that is given to it. If we do what you suggest than
> decoding 1080i and 720p using the same instance of DV decoder
> will not be possible. Or are you suggesting something else?

does it actually work if 1080i and 720p are mixed?
and the table could of course be inited per picture or when a size
change is detected.


> 
> > > @@ -126,10 +132,9 @@ static int dv_extract_audio(uint8_t* fra
> > >              frame += 6 * 80; /* skip DIF segment header */
> > >              if (quant == 1 && i == half_ch) {
> > >                  /* next stereo channel (12bit mode only) */
> > > -                if (!pcm2)
> > > +                pcm = ppcm[ipcm++];
> > > +                if (!pcm)
> > >                      break;
> > > -                else
> > > -                    pcm = pcm2;
> > >              }
> > >  
> > >              /* for each AV sequence */
> >  
> > i cannot comment this because this is not correctly implemented
> > dv_audio_12to16() clearly belongs in a decoder not in the demuxer.
> 
> Well, the new code has nothing to do with 12bit pcm, and in fact,
> the comment should be removed (or updated?) since the audio
> doesn't have to be 12bit pcm in order to have "next" stereo channel.
> 
> What I don't understand though is your statement that dv_audio_12to16()
> belongs to a decoder. Which decoder?

the (currently non existing) 12bit audio DV decoder :)

[...]
-- 
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/20080908/aa45df1c/attachment.pgp>