[FFmpeg-devel] [PATCH] EA TGQ video fixes

[Vitor Sessak]

Reimar D?ffinger wrote:
> On Thu, Apr 16, 2009 at 06:26:27PM +0200, Vitor Sessak wrote:
>> Mike told me that FATE test #262 is not bit exact across platforms. I 
>> see two wrong things with this codec:
>>
>> 1- DSP usage on an unaligned buffer (idct_put(), more precisely). 
>> eatgq2.diff fixes it.
>>
>> 2- "dereferencing type-punned pointer will break strict-aliasing rules" 
>> warning. eatgq1.diff refactor some code to fix it.
> 

> ?? Why don't you just change the arguments from int8_t to uint8_t?

I misread the code. I thought that it wasn't fixed before was because 
this sign was important...

>> > -    DCTELEM block[6][64];
>> > +    DECLARE_ALIGNED_8(DCTELEM, block[6][64]);
>> >  
>> >      mode = bytestream_get_byte((const uint8_t**)bs);
>> >      if (mode>buf_end-*bs) {
> 
> Moving it into the context instead should be more reliable.

Why?

> Also it would be interesting to know on which platforms the results
> differ from which.

Acording to Mike, for a single gcc version on x86_64 and a single gcc 
version on PPC.

> Sorry for the many mails, I tried these patches, this one fixes PPC
> decoding to match:
> Index: eatgq.c
> ===================================================================
> --- eatgq.c     (revision 18539)
> +++ eatgq.c     (working copy)
> @@ -140,7 +140,7 @@
>      }
>  }
>  
> -static void tgq_decode_mb(TgqContext *s, int mb_y, int mb_x, const int8_t **bs, const int8_t *buf_end){
> +static void tgq_decode_mb(TgqContext *s, int mb_y, int mb_x, const uint8_t **bs, const uint8_t *buf_end){
>      int mode;
>      int i;
>      int8_t dc[6];
> @@ -228,7 +228,7 @@
>  
>      for (y=0; y<(avctx->height+15)/16; y++)
>      for (x=0; x<(avctx->width+15)/16; x++)
> -        tgq_decode_mb(s, y, x, (const int8_t**)&buf, (const int8_t*)buf_end);
> +        tgq_decode_mb(s, y, x, &buf, buf_end);
>  
>      *data_size = sizeof(AVFrame);
>      *(AVFrame*)data = s->frame;

I prefer this patch instead of mine, as it is simpler.

> And this one should be correct but makes no difference:
> 
> --- eatgq.c     2009-04-16 17:20:15.000000000 +0000
> +++ eatgq.c.bak 2009-04-16 17:19:11.000000000 +0000
> @@ -42,6 +42,7 @@
>      int width,height;
>      ScanTable scantable;
>      int qtable[64];
> +    DECLARE_ALIGNED_16(DCTELEM, block[6][64]);
>  } TgqContext;
>  
>  static av_cold int tgq_decode_init(AVCodecContext *avctx){
> @@ -144,7 +145,6 @@
>      int mode;
>      int i;
>      int8_t dc[6];
> -    DCTELEM block[6][64];
>  
>      mode = bytestream_get_byte((const uint8_t**)bs);
>      if (mode>buf_end-*bs) {
> @@ -156,8 +156,8 @@
>          GetBitContext gb;
>          init_get_bits(&gb, *bs, mode*8);
>          for(i=0; i<6; i++)
> -            tgq_decode_block(s, block[i], &gb);
> -        tgq_idct_put_mb(s, block, mb_x, mb_y);
> +            tgq_decode_block(s, s->block[i], &gb);
> +        tgq_idct_put_mb(s, s->block, mb_x, mb_y);
>      }else{
>          if (mode==3) {
>              memset(dc, (*bs)[0], 4);

See my question above.

Thanks for taking a look.
-Vitor