[Ffmpeg-devel] THP decoder

[Reimar Döffinger]

Hello,
On Fri, Apr 06, 2007 at 08:05:38PM +0400, Kislyakov Maxim wrote:
> +#define THP_TAG MKTAG('T', 'H', 'P', '\0')

Since it's used in only one place the define seems unneeded to me.

> +    if ((AV_RL32(&p->buf[0]))== THP_TAG) {

superfluous ()

> +        return AVPROBE_SCORE_MAX;
> +    }
> +    else
> +        return 0;

the {} and the else can be left out, too

> +    int *header;
> +    get_buffer(pb, header, 4);/* Skipping magic bytes */

huh? Won't this end up writing the data at some random place?

> +    get_buffer(pb, header, 4);
> +    if ((*header == 0x100) || (*header == 0x110)) {

In addition that is not endian-safe I'd guess.

also, e.g.
> thpDemux->version = get_le32(pb);
> if (thpDemux->version != 0x100 && thpDemux->version != 0x110)
>     return AVERROR_IO;
is simpler, too.
Might be that get_be32 is the right thing though, depends on what kind
of system you are currently.
Also, version is only used in thp_read_header, so there IMO is not
really a reason to put it into thpDemux at all.

> +        if (thpDemux->version == 0x110) {
> +            pb->buf_ptr += 4;
> +        }

That certainly is not right, use url_fskip or a dummy get_be32 or
whatever, but don't fiddle with internals.

> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> +    st->codec->codec_id = CODEC_ID_THP;
> +    st->codec->codec_tag = 0;  /* no fourcc */

Isn't that the default value anyway?

> +    st->codec->sample_rate = thpDemux->fps;

And I don't think you're supposed to set sample_rate for video.

> +        if (!st)
> +        {
> +            return AVERROR_NOMEM;
> +        }

IMO drop the {}, it just wastes screen space. But your decision.
It's also inconsistent because almost everywhere else you put the { on
the same line, not the next.

> +        st->codec->channels = get_be32(pb); /* Number of channels */
> +        st->codec->sample_rate = get_be32(pb); /* Frequency */

That falls in the "useless comments" category for me, really as if you
would do
a = b + c; /* add */

> +        url_fseek( pb, thpDemux->curFrame,SEEK_SET);

This is just an example, there are many other places. Place spaces
consistently. Either always add one after a ',' or never (former
preferred by me). Same for the space after the '(' (though here I would
prefer to have none, though it doesn't matter that much as long as it is
consistent).

> +        if (thpDemux->maxAudioSamples != 0) {
> +            thpDemux->audioSize = get_be32(pb);
> +        }
> +
> +        int read = av_get_packet(pb,pkt,thpDemux->imageSize);
> +        if (read != thpDemux->imageSize) {

breaks compilation with gcc 2.95, variables must be declared at the
start of a block. IIRC doing it like this should also give you a warning
during compilation...

> +        url_fseek(pb,thpDemux->curFrame + thpDemux->imageSize + 16 + thpDemux->audioSize*(thpDemux->indexComponent - 1), SEEK_SET);
> +        int read = av_get_packet(pb,pkt,thpDemux->audioSize);
> +        if (read != thpDemux->audioSize) {

Same here, though I find it funny that you used an extra variable to
keep the if simple (I guess) but you didn't do the same for the
url_fseek line, which really is very long if not to say obfuscated.

> +    ADPCMThpFrameHeader * frameHeader = av_malloc(sizeof(ADPCMThpFrameHeader)); /* Allocating memory for frameHeader */

another really unhelpful comment

> @@ -829,6 +849,7 @@
>      short *samples_end;
>      uint8_t *src;
>      int st; /* stereo */
> +    GetBitContext gb;
> 
>      /* DK3 ADPCM accounting variables */
>      unsigned char last_byte = 0;
> @@ -1111,6 +1132,46 @@
>              buf_size -= 128;
>          }
>          break;
> +    case CODEC_ID_ADPCM_THP:
> +        src += 8;

add a { after the case and put the GetBitContext there. Same for the
other variables you have in that code that are not at a start of a
block.

Greetings,
Reimar D?ffinger