[FFmpeg-devel] [PATCH] Unfinished GSoC qual task (16bit VQA Video Decoder)

Adam Iglewski adam.iglewski
Sat May 2 01:06:24 CEST 2009


Michael Niedermayer pisze:
[...]
>> I would be grateful for hints how to resolve problem with
>> decoding stereo audio (also described in my previous email).
> 
> there are 3 options (in no particular order)
> A. extradata (this requires that there really is a global header in the
>    file for audio)
> B. codec_tag (this requres that there really is a codec tag for the
>    audio)
> C. a seperate codec_id
> 

I think option C is the simplest. It requires only small changes
and adds almost no new code to FFmpeg:

--- ffmpeg/libavcodec/adpcm.c	2009-04-29 23:54:38.000000000 +0200
+++ ffmpeg_work/libavcodec/adpcm.c	2009-05-02 00:30:17.000000000 +0200
@@ -1005,6 +1005,7 @@ static int adpcm_decode_frame(AVCodecCon
          if (cs->step_index < 0) cs->step_index = 0;
          if (cs->step_index > 88) cs->step_index = 88;

+    case CODEC_ID_ADPCM_IMA_WS_V3:
          m= (buf_size - (src - buf))>>st;
          for(i=0; i<m; i++) {
              *samples++ = adpcm_ima_expand_nibble(&c->status[0], src[i] 
& 0x0F, 4);

and

--- ffmpeg/libavformat/westwood.c	2009-04-20 18:35:42.000000000 +0200
+++ ffmpeg_work/libavformat/westwood.c	2009-05-02 00:28:54.000000000 +0200

@@ -254,8 +258,10 @@ static int wsvqa_read_header(AVFormatCon
          st->codec->codec_type = CODEC_TYPE_AUDIO;
          if (AV_RL16(&header[0]) == 1)
              st->codec->codec_id = CODEC_ID_WESTWOOD_SND1;
-        else
+        else if(AV_RL16(&header[14]))
              st->codec->codec_id = CODEC_ID_ADPCM_IMA_WS;
+        else
+            st->codec->codec_id = CODEC_ID_ADPCM_IMA_WS_V3;

plus of course adding codec_id to avcodec.h etc.
Is this option acceptable?

> 
> [...]
> 
>> +                /* Write block number (Val & 0xff) and then write Count blocks
>> +                 * getting their indexes by reading next Count bytes from
>> +                 * the VPTR chunk. Count is (((Val/256) & 0x1f)+1)*2
> 
> defining what count is makes no sense its clear in the code already
> actually i think the whole comments are useless
> 

I agree. I just added them when I started working on this.
All removed.
.
> 
>> +                 */
>> +                case 0x4000:
>> +                    vector_index = (vptr_action_code & 0xff) << index_shift;
> 
>> +                    blocks_done = ((((vptr_action_code>>8) & 0x1f)+1) << 1)+1;
> 
> this can be simplified
> 
> 
>> +                    break;
>> +
> 
>> +                /* Write block (Val & 0x1fff).*/
>> +                case 0x6000:
>> +                    vector_index = (vptr_action_code & 0x1fff) << index_shift;
>> +                    blocks_done=1;
>> +                    break;
>> +
>> +                /* Write block (Val & 0x1fff) Count times.
>> +                 * Count is the next byte from the VPTR chunk
>> +                 */
>> +                case 0xa000:
>> +                    vector_index = (vptr_action_code & 0x1fff) << index_shift;
>> +                    blocks_done = *src++;
>> +                    break;
> 
> can be merged
> 
> 
>> +
>> +                default:
>> +                    av_log(s->avctx, AV_LOG_ERROR, " unknown action code in VPTR chunk (%x)\n",(vptr_action_code & 0xe000));
>> +                    return;
>> +            }
>> +
>> +            if(pixels + s->vector_height * stride + blocks_done * block_inc > frame_end) {
>> +                av_log(s->avctx, AV_LOG_ERROR, " too many blocks in frame.\n");
>> +                return;
>> +            }
>> +
>> +            for(i=0; i < blocks_done; i++) {
>> +                if(i && ((vptr_action_code & 0xe000) == 0x4000)) {
> 
>> +                    vector_index = *src++;
>> +                    vector_index <<= index_shift;
> 
> can be merged
> 

All above - updated patch attached.

> [...]
>> @@ -561,6 +698,20 @@ static void vqa_decode_chunk(VqaContext 
>>              s->partial_countdown = s->partial_count;
>>          }
>>      }
>> +
>> +    if(vptr_chunk != -1) {
>> +        chunk_size = AV_RB32(&s->buf[vptr_chunk + 4]);
> 
> validity check missing
> 
> 
>> +        vptr_chunk += CHUNK_PREAMBLE_SIZE;
>> +        vqa_decode_hc_video_chunk(s,&s->buf[vptr_chunk],chunk_size);
>> +    }
>> +
>> +    if(vprz_chunk != -1) {
>> +        chunk_size = AV_RB32(&s->buf[vprz_chunk + 4]);
> 
> same
> 
> also the code likely can be factorized
> 

By validity check you mean checking that AV_RB32 is not
reading outside s->buf or checking if chunk_size isn't
bigger that s->size ? I'm asking because there is a lot
of similar code in this function/file and there are
no checks like this.

As for refactoring this is part of the function that you
wanted to be rewritten to process chunks as needed.
So maybe for now this could be as is?

Adam


-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_vqa_hc.diff
Type: text/x-diff
Size: 9129 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090502/81eb8a53/attachment.diff>



More information about the ffmpeg-devel mailing list