[Ffmpeg-devel] I'm giving up

Panagiotis Issaris takis.issaris
Thu Dec 7 17:14:16 CET 2006 

Hi Michael,

On Wed, 2006-12-06 at 15:25 +0100, Michael Niedermayer wrote:
>[...]
> > +static uint8_t *h264_write_nal_unit(int nal_ref_idc, int nal_unit_type, uint8_t *dest, int *destsize,
> > +                          PutBitContext *b2)
> > +{
> > +    PutBitContext b;
> > +    int i, destpos, rbsplen, escape_count;
> > +    uint8_t *rbsp;
> > +
> > +    // Align b2 on a byte boundary
> > +
> > +    align_put_bits(b2);
> 
> is the rbsp trailing stuff correct? shouldnt there be a put_bits(1,1) ?
Yes, but we had the put_bits(1,1) call in the calling context. For
end-of-stream NAL units, the rbsp should be empty, so in that case we
did not add the stopbit.

How about something like this? (Tested and works, but the reference
decoder also decodes it if the extra bit is there.)

    ...
    uint8_t *rbsp;
    
    if (nal_unit_type != NAL_END_STREAM)
        put_bits(b2,1,1); // rbsp_stop_bit

    // Align b2 on a byte boundary
    align_put_bits(b2);
    ...

> > +    rbsplen = put_bits_count(b2)/8;
> > +    flush_put_bits(b2);
> > +    rbsp = b2->buf;
>[...]

> the {} placement is inconsistant
I'll fix this.

> except these iam fine with commiting h264enc.c
Great! :)

> [...]
> > +static void h264_dct_c(DCTELEM block[4][4])
> > +{
> > +    DCTELEM pieces[4][4];
> > +
> > +    pieces[0][0] = block[0][0]+block[1][0]+block[2][0]+block[3][0];
> > +    pieces[0][1] = block[0][1]+block[1][1]+block[2][1]+block[3][1];
> > +    pieces[0][2] = block[0][2]+block[1][2]+block[2][2]+block[3][2];
> > +    pieces[0][3] = block[0][3]+block[1][3]+block[2][3]+block[3][3];
> > +
> > +    pieces[1][0] = (block[0][0]<<1)+block[1][0]-block[2][0]-(block[3][0]<<1);
> > +    pieces[1][1] = (block[0][1]<<1)+block[1][1]-block[2][1]-(block[3][1]<<1);
> > +    pieces[1][2] = (block[0][2]<<1)+block[1][2]-block[2][2]-(block[3][2]<<1);
> > +    pieces[1][3] = (block[0][3]<<1)+block[1][3]-block[2][3]-(block[3][3]<<1);
> > +
> > +    pieces[2][0] = block[0][0]-block[1][0]-block[2][0]+block[3][0];
> > +    pieces[2][1] = block[0][1]-block[1][1]-block[2][1]+block[3][1];
> > +    pieces[2][2] = block[0][2]-block[1][2]-block[2][2]+block[3][2];
> > +    pieces[2][3] = block[0][3]-block[1][3]-block[2][3]+block[3][3];
> > +
> > +    pieces[3][0] = block[0][0]-(block[1][0]<<1)+(block[2][0]<<1)-block[3][0];
> > +    pieces[3][1] = block[0][1]-(block[1][1]<<1)+(block[2][1]<<1)-block[3][1];
> > +    pieces[3][2] = block[0][2]-(block[1][2]<<1)+(block[2][2]<<1)-block[3][2];
> > +    pieces[3][3] = block[0][3]-(block[1][3]<<1)+(block[2][3]<<1)-block[3][3];
> > +
> > +    block[0][0] = pieces[0][0]+pieces[0][1]+pieces[0][2]+pieces[0][3];
> > +    block[0][1] = pieces[1][0]+pieces[1][1]+pieces[1][2]+pieces[1][3];
> > +    block[0][2] = pieces[2][0]+pieces[2][1]+pieces[2][2]+pieces[2][3];
> > +    block[0][3] = pieces[3][0]+pieces[3][1]+pieces[3][2]+pieces[3][3];
> > +
> > +    block[1][0] = (pieces[0][0] << 1)+pieces[0][1]-pieces[0][2]-(pieces[0][3]<<1);
> > +    block[1][1] = (pieces[1][0] << 1)+pieces[1][1]-pieces[1][2]-(pieces[1][3]<<1);
> > +    block[1][2] = (pieces[2][0] << 1)+pieces[2][1]-pieces[2][2]-(pieces[2][3]<<1);
> > +    block[1][3] = (pieces[3][0] << 1)+pieces[3][1]-pieces[3][2]-(pieces[3][3]<<1);
> > +
> > +    block[2][0] = pieces[0][0]-pieces[0][1]-pieces[0][2]+pieces[0][3];
> > +    block[2][1] = pieces[1][0]-pieces[1][1]-pieces[1][2]+pieces[1][3];
> > +    block[2][2] = pieces[2][0]-pieces[2][1]-pieces[2][2]+pieces[2][3];
> > +    block[2][3] = pieces[3][0]-pieces[3][1]-pieces[3][2]+pieces[3][3];
> > +
> > +    block[3][0] = pieces[0][0]-(pieces[0][1]<<1)+(pieces[0][2]<<1)-pieces[0][3];
> > +    block[3][1] = pieces[1][0]-(pieces[1][1]<<1)+(pieces[1][2]<<1)-pieces[1][3];
> > +    block[3][2] = pieces[2][0]-(pieces[2][1]<<1)+(pieces[2][2]<<1)-pieces[2][3];
> > +    block[3][3] = pieces[3][0]-(pieces[3][1]<<1)+(pieces[3][2]<<1)-pieces[3][3];
> 
> theres are alot of redundant operations in the above, these should be
> simplified
I haven't spotted the redundant operations yet, but I'll go over them
tomorrow (or this evening).


> [...]
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index 03c1ae4..ac369eb 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -86,6 +86,7 @@ OBJS-$(CONFIG_GIF_ENCODER)             +
> >  OBJS-$(CONFIG_H261_DECODER)            += h261.o
> >  OBJS-$(CONFIG_H261_ENCODER)            += h261.o
> >  OBJS-$(CONFIG_H264_DECODER)            += h264.o
> > +OBJS-$(CONFIG_H264_ENCODER)            += h264enc.o h264cavlc.o h264dsp.o
> >  OBJS-$(CONFIG_HUFFYUV_DECODER)         += huffyuv.o
> >  OBJS-$(CONFIG_HUFFYUV_ENCODER)         += huffyuv.o
> >  OBJS-$(CONFIG_IDCIN_DECODER)           += idcinvideo.o
> 
> this can be commited if it does not break compilation
By adding a stub for h264cavlc.c or by disabling compilation by default?


> > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> > index 9678f6b..c2c3bbb 100644
> > --- a/libavcodec/allcodecs.c
> > +++ b/libavcodec/allcodecs.c
> > @@ -75,7 +75,7 @@ void avcodec_register_all(void)
> >      REGISTER_ENCDEC (H263, h263);
> >      REGISTER_DECODER(H263I, h263i);
> >      REGISTER_ENCODER(H263P, h263p);
> > -    REGISTER_DECODER(H264, h264);
> > +    REGISTER_ENCDEC(H264, h264);
> >      REGISTER_ENCDEC (HUFFYUV, huffyuv);
> 
> this can be commited if it does not break compilation
Same here: By disabling default compilation of the encoder or by adding
a AVCodec h264_encoder to h264enc.c?


> >      REGISTER_DECODER(IDCIN, idcin);
> >      REGISTER_DECODER(INDEO2, indeo2);
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 96ea77a..4457969 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -149,6 +149,7 @@ enum CodecID {
> >      CODEC_ID_TIERTEXSEQVIDEO,
> >      CODEC_ID_TIFF,
> >      CODEC_ID_GIF,
> > +    CODEC_ID_FFH264,
> 
> ok can be commited
Done.


  
> >      /* various pcm "codecs" */
> >      CODEC_ID_PCM_S16LE= 0x10000,
> > diff --git a/libavcodec/dsputil.c b/libavcodec/dsputil.c
> > index 51eddbc..84f1bfb 100644
> > --- a/libavcodec/dsputil.c
> > +++ b/libavcodec/dsputil.c
> > @@ -2549,6 +2549,11 @@ void ff_put_vc1_mspel_mc00_c(uint8_t *ds
> >  }
> >  #endif /* CONFIG_VC1_DECODER||CONFIG_WMV3_DECODER */
> >  
> > +#if defined(CONFIG_H264_ENCODER)
> > +/* H264 specific */
> > +void ff_h264dsp_init(DSPContext* c, AVCodecContext *avctx);
> > +#endif /* CONFIG_H264_ENCODER */
> 
> this can be commited as soon as theres a ff_h264dsp_init() and as long
> as doing so doenst break anything
Okay, I'll remember this. As soon as I get the H.264 transform in good
shape, I'll add a dsp init function and commit this.


> [...]
> 
> > @@ -383,6 +385,14 @@ typedef struct DSPContext {
> >      void (*h264_idct8_add)(uint8_t *dst, DCTELEM *block, int stride);
> >      void (*h264_idct_dc_add)(uint8_t *dst, DCTELEM *block, int stride);
> >      void (*h264_idct8_dc_add)(uint8_t *dst, DCTELEM *block, int stride);
> > +    void (*h264_dct)(DCTELEM block[4][4]);
> 
> iam ok with adding this one anytime
Done.


> > +    void (*h264_idct_notranspose_add)(uint8_t *dst, DCTELEM *block, int stride);
> 
> > +    void (*h264_hadamard_mult4x4)(DCTELEM Y[4][4]);
> 
> mult4x4 vs. mult_4x4 ?
Fixed by renaming more consistently to _XxX.

> [...]
> > +
> > +const uint8_t ff_div6[52]={
> > +0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 5, 6, 6, 6, 6, 6, 6, 7, 7, 7, 7, 7, 7, 8, 8, 8, 8,
> > +};
> 
> can be commited with all the needed renamings
Done.



> > [...]
> > -        block[2*4 + i]=   z0 -   z1;
> > -        block[3*4 + i]=   z3 - 2*z2;
> > -    }
> > -}
> > -#endif
> 
> ok, can be commited
Done.

With friendly regards,
Takis
-- 
vCard: http://www.issaris.org/pi.vcf
Public key: http://www.issaris.org/pi.key

More information about the ffmpeg-devel mailing list