[FFmpeg-devel] [PATCH] VP8 de/encode via libvpx

[James Zern]

On Wed, May 19, 2010 at 16:40, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> Hello,
> From the decoder patch (note that at least for me review/quoting would be far
> easier with text/plain attachment type):
Sorry, didn't pay attention to the attachment type through gmail.
>
>> + ?const uint8_t* const buf = avpkt->data;
>> + ?const int buf_size = avpkt->size;
>> + ?vp8dctx_t* const ctx = avctx->priv_data;
>> + ?AVFrame* const picture = data;
>
> These const do nothing but making code changes harder, and we don't
> use them this way anywhere else. Also in FFmpeg the * goes with the variable name,
> not the type.
OK. The next patchset will contain the style modifications, though I
may do the encoder/decoder in 2 separate passes.

> I also fail to see the point of the extra buf and buf_size variables,
> they seem to be used only once.
Hold overs from the api change, these have had a fairly long life.

>
>> + ?if(vpx_codec_decode(&ctx->decoder,buf,buf_size,NULL,0)!=VPX_CODEC_OK) {
>
> To handle codec delay, buf == NULL and buf_size == 0 is valid in FFmpeg,
> is using it this way also part of the VPX API and has the same meaning?
> Otherwise it would be better to handle it specially.
No that will actually get you the last frame, though I thought NULL
buf would only occur if _DELAY was set.

> Also, does the decoder somehow tell how much of the data it actually used?
Frame based so it expects to consume the entire buffer.

>
>> + ? ?picture->data[0] = img->planes[0];
>> + ? ?picture->data[1] = img->planes[1];
>> + ? ?picture->data[2] = img->planes[2];
>> + ? ?picture->data[3] = img->planes[3];
>> + ? ?picture->linesize[0] = img->stride[0];
>> + ? ?picture->linesize[1] = img->stride[1];
>> + ? ?picture->linesize[2] = img->stride[2];
>> + ? ?picture->linesize[3] = img->stride[3];
>
> And while as someone else mentioned you might not have to copy the [3] entries,
> I think they should initialized.
Will do.