On 01/16/2012 09:08 PM, Laurentiu Ion wrote:
> ---
> libavcodec/dxa.c | 167 +++++++++++++++++++++++++++++------------------------
> 1 files changed, 91 insertions(+), 76 deletions(-)
>
> diff --git a/libavcodec/dxa.c b/libavcodec/dxa.c
> index 97b912a..83cf6ea 100644
> --- a/libavcodec/dxa.c
> +++ b/libavcodec/dxa.c
> @@ -29,6 +29,7 @@
>
> #include "libavutil/intreadwrite.h"
> #include "avcodec.h"
> +#include "bytestream.h"
>
> #include <zlib.h>
>
> @@ -39,36 +40,51 @@ typedef struct DxaDecContext {
> AVCodecContext *avctx;
> AVFrame pic, prev;
>
> - int dsize;
> + unsigned long decomp_size;
> + int allocated_decomp_size;
> uint8_t *decomp_buf;
> uint32_t pal[256];
> + GetByteContext g;
> } DxaDecContext;
>
> static const int shift1[6] = { 0, 8, 8, 8, 4, 4 };
> static const int shift2[6] = { 0, 0, 8, 4, 0, 4 };
>
> -static int decode_13(AVCodecContext *avctx, DxaDecContext *c, uint8_t* dst,
> uint8_t *src, uint8_t *ref)
> +static int decode_13(DxaDecContext *c, uint8_t* dst, uint8_t *src, uint8_t
> *ref)
> {
> - uint8_t *code, *data, *mv, *msk, *tmp, *tmp2;
> + GetByteContext code, data, mv, msk;
> + uint8_t tmp3;
> + int datatmp;
> + uint32_t datatmp32;
> + int t1, t2;
> + uint8_t t[4];
> + uint8_t *code_buf, *data_buf, *mv_buf, *msk_buf, *tmp, *tmp2;
> int i, j, k;
> int type, x, y, d, d2;
> int stride = c->pic.linesize[0];
> uint32_t mask;
>
> - code = src + 12;
> - data = code + ((avctx->width * avctx->height) >> 4);
> - mv = data + AV_RB32(src + 0);
> - msk = mv + AV_RB32(src + 4);
> + //bytestream2_init(&code, src + 12, decomp_size);
> + code_buf = src + 12;
> + data_buf = code_buf + ((c->avctx->width * c->avctx->height) >> 4);
> + mv_buf = data_buf + AV_RB32(src + 0);
> + msk_buf = mv_buf + AV_RB32(src + 4);
> +
> + bytestream2_init(&code, code_buf, c->decomp_size);
> + bytestream2_init(&data, data_buf, c->decomp_size);
> + bytestream2_init(&mv, mv_buf, c->decomp_size);
> + bytestream2_init(&msk, msk_buf, c->decomp_size);
The initialized size is wrong for all of those. I would also recommend
not having a separate GetByteContext for "code" and just use c->g for
that. You don't need to have temporary buffer variables. Your code is
not any safer than the existing code because the offsets are not
validated, nor is the remaining size. A safer approach might be
something like:
mv_offset = bytestream2_get_be32(&s->g);
msk_offset = bytestream2_get_be32(&s->g);
bytestream2_skip(&s->g, 12);
data = s->g;
bytestream2_skip(&data, (avctx->width * avctx->height) >> 4);
mv = data;
bytestream2_skip(&mv, mv_offset);
msk = mv
bytestream2_skip(&msk, msk_offset);
>
> - for(j = 0; j < avctx->height; j += 4){
> - for(i = 0; i < avctx->width; i += 4){
> + for(j = 0; j < c->avctx->height; j += 4){
> + for(i = 0; i < c->avctx->width; i += 4){
> tmp = dst + i;
> tmp2 = ref + i;
> - type = *code++;
> + type = bytestream2_get_byte(&code);
> switch(type){
> case 4: // motion compensation
> - x = (*mv) >> 4; if(x & 8) x = 8 - x;
> - y = (*mv++) & 0xF; if(y & 8) y = 8 - y;
> + tmp3 = bytestream2_get_byte(&mv);
> + x = (tmp3) >> 4; if(x & 8) x = 8 - x;
> + y = (tmp3) & 0xF; if(y & 8) y = 8 - y;
please go ahead and separate these lines
> tmp2 += x + y*stride;
> case 0: // skip
> case 5: // skip in method 12
> @@ -85,17 +101,16 @@ static int decode_13(AVCodecContext *avctx,
> DxaDecContext *c, uint8_t* dst, uint
> case 13:
> case 14:
> case 15:
> - if(type == 1){
> - mask = AV_RB16(msk);
> - msk += 2;
> - }else{
> + if(type == 1)
> + mask = bytestream2_get_be16(&msk);
> + else{
> + tmp3 = bytestream2_get_byte(&msk);
> type -= 10;
> - mask = ((msk[0] & 0xF0) << shift1[type]) | ((msk[0] &
> 0xF) << shift2[type]);
> - msk++;
> + mask = ((tmp3 & 0xF0) << shift1[type]) | ((tmp3 & 0xF)
> << shift2[type]);
> }
> for(y = 0; y < 4; y++){
> for(x = 0; x < 4; x++){
> - tmp[x] = (mask & 0x8000) ? *data++ : tmp2[x];
> + tmp[x] = (mask & 0x8000) ?
> bytestream2_get_byte(&data) : tmp2[x];
> mask <<= 1;
> }
> tmp += stride;
> @@ -103,29 +118,29 @@ static int decode_13(AVCodecContext *avctx,
> DxaDecContext *c, uint8_t* dst, uint
> }
> break;
> case 2: // fill block
> + tmp3 = bytestream2_get_byte(&data);
> for(y = 0; y < 4; y++){
> - memset(tmp, data[0], 4);
> + memset(tmp, tmp3, 4);
> tmp += stride;
> }
> - data++;
> break;
> case 3: // raw block
> for(y = 0; y < 4; y++){
> - memcpy(tmp, data, 4);
> - data += 4;
> + bytestream2_get_buffer(&data, tmp, 4);
> tmp += stride;
> }
> break;
> case 8: // subblocks - method 13 only
> - mask = *msk++;
> + mask = bytestream2_get_byte(&msk);
> for(k = 0; k < 4; k++){
> d = ((k & 1) << 1) + ((k & 2) * stride);
> d2 = ((k & 1) << 1) + ((k & 2) * stride);
> tmp2 = ref + i + d2;
> switch(mask & 0xC0){
> case 0x80: // motion compensation
> - x = (*mv) >> 4; if(x & 8) x = 8 - x;
> - y = (*mv++) & 0xF; if(y & 8) y = 8 - y;
> + tmp3 = bytestream2_get_byte(&mv);
> + x = (tmp3) >> 4; if(x & 8) x = 8 - x;
> + y = (tmp3) & 0xF; if(y & 8) y = 8 - y;
same here
> tmp2 += x + y*stride;
> case 0x00: // skip
> tmp[d + 0 ] = tmp2[0];
> @@ -134,51 +149,53 @@ static int decode_13(AVCodecContext *avctx,
> DxaDecContext *c, uint8_t* dst, uint
> tmp[d + 1 + stride] = tmp2[1 + stride];
> break;
> case 0x40: // fill
> - tmp[d + 0 ] = data[0];
> - tmp[d + 1 ] = data[0];
> - tmp[d + 0 + stride] = data[0];
> - tmp[d + 1 + stride] = data[0];
> - data++;
> + datatmp = bytestream2_get_be16(&data);
> + tmp[d ] = datatmp;
> + tmp[d + stride] = datatmp;
not quite right. the current code duplicates 1 byte and skips 1 byte.
your patch uses both bytes.
> break;
> case 0xC0: // raw
> - tmp[d + 0 ] = *data++;
> - tmp[d + 1 ] = *data++;
> - tmp[d + 0 + stride] = *data++;
> - tmp[d + 1 + stride] = *data++;
> + tmp[d ] = bytestream2_get_be16(&data);
> + tmp[d + stride] = bytestream2_get_be16(&data);
> break;
> }
> mask <<= 2;
> }
> break;
> case 32: // vector quantization - 2 colors
> - mask = AV_RB16(msk);
> - msk += 2;
> + mask = bytestream2_get_be16(&msk);
> + datatmp = bytestream2_get_be16(&data);
> + t[0] = datatmp >> 8;
> + t[1] = datatmp & 0xFF;
instead of the shift/mask why not just do:
t[0] = bytestream2_get_byte(&data);
t[1] = bytestream2_get_byte(&data);
> for(y = 0; y < 4; y++){
> for(x = 0; x < 4; x++){
> - tmp[x] = data[mask & 1];
> + tmp[x] = t[mask & 1];
> mask >>= 1;
> }
> tmp += stride;
> tmp2 += stride;
> }
> - data += 2;
> break;
> case 33: // vector quantization - 3 or 4 colors
> case 34:
> - mask = AV_RB32(msk);
> - msk += 4;
> + mask = bytestream2_get_be32(&msk);
> + datatmp32 = bytestream2_get_be32(&data);
> + t1 = datatmp32 >> 16;
> + t[0] = t1 >> 8;
> + t[1] = t1 & 0xFF;
> + t2 = datatmp32 & 0xFFFF;
> + t[2] = t2 >> 8;
> + t[3] = t2 & 0xFF;
same concept here. it can probably be done more simply.
> for(y = 0; y < 4; y++){
> for(x = 0; x < 4; x++){
> - tmp[x] = data[mask & 3];
> + tmp[x] = t[mask & 3];
> mask >>= 2;
> }
> tmp += stride;
> tmp2 += stride;
> }
> - data += type - 30;
> break;
> default:
> - av_log(avctx, AV_LOG_ERROR, "Unknown opcode %d\n", type);
> + av_log(c->avctx, AV_LOG_ERROR, "Unknown opcode %d\n", type);
> return -1;
> }
> }
> @@ -188,31 +205,23 @@ static int decode_13(AVCodecContext *avctx,
> DxaDecContext *c, uint8_t* dst, uint
> return 0;
> }
>
> -static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> AVPacket *avpkt)
> +static int decode_frame(AVCodecContext *avctx,
> + void *data, int *data_size, AVPacket *avpkt)
> {
> - const uint8_t *buf = avpkt->data;
> - int buf_size = avpkt->size;
> DxaDecContext * const c = avctx->priv_data;
> uint8_t *outptr, *srcptr, *tmpptr;
> - unsigned long dsize;
> int i, j, compr;
> int stride;
> - int orig_buf_size = buf_size;
> int pc = 0;
>
> - /* make the palette available on the way out */
> - if(buf[0]=='C' && buf[1]=='M' && buf[2]=='A' && buf[3]=='P'){
> - int r, g, b;
> + bytestream2_init(&c->g, avpkt->data, avpkt->size);
>
> - buf += 4;
> - for(i = 0; i < 256; i++){
> - r = *buf++;
> - g = *buf++;
> - b = *buf++;
> - c->pal[i] = (r << 16) | (g << 8) | b;
> - }
> + /* make the palette available on the way out */
> + if(bytestream2_peek_le32(&c->g) == MKTAG('C','M','A','P')) {
> + bytestream2_skip(&c->g, 4);
> + for(i = 0; i < 256; i++)
> + c->pal[i] = bytestream2_get_be24(&c->g);
> pc = 1;
> - buf_size -= 768+4;
> }
>
> if(avctx->get_buffer(avctx, &c->pic) < 0){
> @@ -227,16 +236,24 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *data_size, AVPac
> tmpptr = c->prev.data[0];
> stride = c->pic.linesize[0];
>
> - if(buf[0]=='N' && buf[1]=='U' && buf[2]=='L' && buf[3]=='L')
> + if(bytestream2_get_le32(&c->g) == MKTAG('N','U','L','L')) {
> compr = -1;
> - else
> - compr = buf[4];
> + }
> + else {
> + compr = bytestream2_get_byte(&c->g);
> + bytestream2_skip(&c->g, 4);
> + }
>
> - dsize = c->dsize;
> - if((compr != 4 && compr != -1) && uncompress(c->decomp_buf, &dsize, buf
> + 9, buf_size - 9) != Z_OK){
> - av_log(avctx, AV_LOG_ERROR, "Uncompress failed!\n");
> - return -1;
> + if((compr != 4 && compr != -1)){
> + c->decomp_size = c->allocated_decomp_size;
> + if(uncompress(c->decomp_buf, &c->decomp_size,
> + avpkt->data + bytestream2_tell(&c->g),
> + bytestream2_get_bytes_left(&c->g)) != Z_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Uncompress failed!\n");
> + return -1;
> + }
> }
> + bytestream2_init(&c->g, c->decomp_buf, c->decomp_size);
> switch(compr){
> case -1:
> c->pic.key_frame = 0;
> @@ -258,22 +275,21 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *data_size, AVPac
> for(j = 0; j < avctx->height; j++){
> if(compr & 1){
> for(i = 0; i < avctx->width; i++)
> - outptr[i] = srcptr[i] ^ tmpptr[i];
> + outptr[i] = bytestream2_get_byte(&c->g) ^ tmpptr[i];
> tmpptr += stride;
> }else
> - memcpy(outptr, srcptr, avctx->width);
> + bytestream2_get_buffer(&c->g, outptr, avctx->width);
> outptr += stride;
> - srcptr += avctx->width;
> }
> break;
> case 12: // ScummVM coding
> case 13:
> c->pic.key_frame = 0;
> c->pic.pict_type = AV_PICTURE_TYPE_P;
> - decode_13(avctx, c, c->pic.data[0], srcptr, c->prev.data[0]);
> + decode_13(c, c->pic.data[0], srcptr, c->prev.data[0]);
you can avoid passing srcptr here.
> break;
> default:
> - av_log(avctx, AV_LOG_ERROR, "Unknown/unsupported compression type
> %d\n", buf[4]);
> + av_log(avctx, AV_LOG_ERROR, "Unknown/unsupported compression type
> %d\n", compr);
> return -1;
> }
>
> @@ -285,7 +301,7 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *data_size, AVPac
> *(AVFrame*)data = c->prev;
>
> /* always report that the buffer was completely consumed */
> - return orig_buf_size;
> + return avpkt->size;
> }
>
> static av_cold int decode_init(AVCodecContext *avctx)
> @@ -295,8 +311,8 @@ static av_cold int decode_init(AVCodecContext *avctx)
> c->avctx = avctx;
> avctx->pix_fmt = PIX_FMT_PAL8;
>
> - c->dsize = avctx->width * avctx->height * 2;
> - if((c->decomp_buf = av_malloc(c->dsize)) == NULL) {
> + c->allocated_decomp_size = avctx->width * avctx->height * 2;
> + if((c->decomp_buf = av_malloc(c->allocated_decomp_size)) == NULL) {
> av_log(avctx, AV_LOG_ERROR, "Can't allocate decompression
> buffer.\n");
> return -1;
> }
> @@ -328,4 +344,3 @@ AVCodec ff_dxa_decoder = {
> .capabilities = CODEC_CAP_DR1,
> .long_name = NULL_IF_CONFIG_SMALL("Feeble Files/ScummVM DXA"),
> };
> -
The rest looks basically ok. There are still quite a few cosmetic
issues, but fix the code first, then I can do a cosmetic review.
Thanks,
Justin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
