[FFmpeg-devel] [FFmpeg-cvslog] avcodec: add Apple Pixlet decoder

[Andreas Cadhalpun]

On 23.12.2016 09:17, Paul B Mahol wrote:
> On 12/23/16, Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>> On 22.12.2016 22:52, Paul B Mahol wrote:
>>> +        xflag = flag + cnt1;
>>> +        yflag = xflag;
>>> +
>>> +        if (flag + cnt1 == 0) {
>>> +            value = 0;
>>> +        } else {
>>> +            xflag &= 1u;
>>> +            tmp = c * ((yflag + 1) >> 1) + (c >> 1);
>>
>> This can overflow.
> 
> And?

That is undefined behavior. It looks like at least a cast to int64_t is missing.

>>> +            value = xflag + (tmp ^ -xflag);
>>> +        }
>>> +
>>> +        i++;
>>> +        dst[j++] = value;
>>> +        if (j == width) {
>>> +            j = 0;
>>> +            dst += stride;
>>> +        }
>>> +        state += d * yflag - (d * state >> 8);
>>
>> This can overflow, too.
> 
> And?

Same as above.

>> [...]
>>> +static void postprocess_chroma(AVFrame *frame, int w, int h, int depth)
>>> +{
>>> +    uint16_t *dstu = (uint16_t *)frame->data[1];
>>> +    uint16_t *dstv = (uint16_t *)frame->data[2];
>>> +    int16_t *srcu  = (int16_t *)frame->data[1];
>>> +    int16_t *srcv  = (int16_t *)frame->data[2];
>>> +    ptrdiff_t strideu = frame->linesize[1] / 2;
>>> +    ptrdiff_t stridev = frame->linesize[2] / 2;
>>> +    const int add = 1 << (depth - 1);
>>> +    const int shift = 16 - depth;
>>> +    int i, j;
>>> +
>>> +    for (j = 0; j < h; j++) {
>>> +        for (i = 0; i < w; i++) {
>>> +            dstu[i] = (add + srcu[i]) << shift;
>>> +            dstv[i] = (add + srcv[i]) << shift;
>>
>> These result in undefined shifts, since the value to be shifted can be
>> negative.
> 
> OK.

Making add unsigned would fix that.

>>> +        }
>>> +        dstu += strideu;
>>> +        dstv += stridev;
>>> +        srcu += strideu;
>>> +        srcv += stridev;
>>> +    }
>>> +}
>>> +
>>> +static int decode_plane(AVCodecContext *avctx, int plane, AVPacket
>>> *avpkt, AVFrame *frame)
>>> +{
>>> +    PixletContext *ctx = avctx->priv_data;
>>> +    ptrdiff_t stride = frame->linesize[plane] / 2;
>>> +    unsigned shift = plane > 0;
>>> +    int16_t *dst;
>>> +    int i, ret;
>>> +
>>> +    for (i = ctx->levels - 1; i >= 0; i--) {
>>> +        ctx->scaling[plane][H][i] = 1000000. /
>>> sign_extend(bytestream2_get_be32(&ctx->gb), 32);
>>> +        ctx->scaling[plane][V][i] = 1000000. /
>>> sign_extend(bytestream2_get_be32(&ctx->gb), 32);
>>
>> Here the denominators can be zero.
> 
> And?

That is undefined behavior.

>>> +    *got_frame = 1;
>>> +
>>> +    return pktsize;
>>
>> Since this is a video decoder, this should always return the full
>> avpkt->size.
> 
> Wrong. It returns what it consumes.

Video decoders are expected to consume the full packet and the new decoding API does that.
So not returning the full packet size results in a behavior difference between the new
and the old API, which is bad. Why would you want that?

Best regards,
Andreas