On 23.12.2016 09:17, Paul B Mahol wrote:
> On 12/23/16, Andreas Cadhalpun <andreas.cadhal...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
