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

[Roman Shaposhnik]

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

> > +
> > +    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?

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

Thanks,
Roman.