av_packet_ref incorrectly processes packets with offset between data and buffer

[mrlika]

As example aac_adtstoasc bitstream filter returns pakcets with pkt->data != pkt->buf->data (i.e. with offset) when I started to use new av_bsf_* API.

Then if you call av_packet_ref on such packet you will get incorrect dst packet because it just sets dst->data = dst->buf->data in the code:

int av_packet_ref(AVPacket *dst, const AVPacket *src)
{
    int ret;

    ret = av_packet_copy_props(dst, src);
    if (ret < 0)
        return ret;

    if (!src->buf) {
        ret = packet_alloc(&dst->buf, src->size);
        if (ret < 0)
            goto fail;
        memcpy(dst->buf->data, src->data, src->size);
    } else {
        dst->buf = av_buffer_ref(src->buf);
        if (!dst->buf) {
            ret = AVERROR(ENOMEM);
            goto fail;
        }
    }

    dst->size = src->size;
    dst->data = dst->buf->data;
    return 0;
fail:
    av_packet_free_side_data(dst);
    return ret;
}

For example FLV muxer calls av_packet_ref internally that causes errors when processing packets from aac_adtstoasc bitstream filter.

[Carl Eugen Hoyos]

How can I reproduce the issue?

[mrlika]

Replying to cehoyos:

How can I reproduce the issue?

I don't know how to reproduce the issue using ffmpeg currently because in trunc it still uses already deprecated av_bitstream_* API.

Once ffmpeg switches to new av_bsf_* API you will get errors with aac_adtstoasc filter -> FLV muxer flow.

To reproduce by coding you can create valid packet with pkt->data that has offset to pkt->buf->data and pass it to av_packet_ref. dst packet will not have the offset as you can see ffrom source code of av_packet_ref above.

[Carl Eugen Hoyos]

Replying to mrlika:

Replying to cehoyos:

How can I reproduce the issue?

I don't know how to reproduce the issue using ffmpeg currently because in trunc it still uses already deprecated av_bitstream_* API.

Can you provide source code that allows to reproduce the issue?

[gjdfgh]

This looks like a pretty bad bug, and the code looks incorrect to me. Just write a patch that fixes this issue (should be simple), and send it to the mailing list.

[mrlika]

The fix is just to change

dst->data = dst->buf->data;

to

dst->data = dst->buf->data + (src->data - src->buf->data);

Probably some src->data and src->buf->data validations are required.

By the way because of this bug AVPacket *av_packet_clone(AVPacket *src) that calls av_packt_ref internally clones incorrectly packets with offset.The offset is lost.

[mrlika]

Source code to reproduce:

static void testcase_for_av_packet_ref(void) {
    AVPacket *src = av_packet_alloc();
    AVPacket *dst = av_packet_alloc();

    av_new_packet(src, 32); /* Allocate payload 32 bytes */

    assert(src->data == src->buf->data);
    assert(src->size == 32);
    assert(src->buf->size == 32 + AV_INPUT_BUFFER_PADDING_SIZE);

    /* Doing payload offset by 16 bytes. Buffer remains the same */
    src->data += 16;
    src->size -= 16;

    av_packet_ref(dst, src);

    assert(dst->size == src->size);
    assert(dst->data == src->data); /* This fails */
}

static void testcase_for_av_packet_clone(void) {
    AVPacket *src = av_packet_alloc();
    AVPacket *dst;

    av_new_packet(src, 32); /* Allocate payload 32 bytes */

    assert(src->data == src->buf->data);
    assert(src->size == 32);
    assert(src->buf->size == 32 + AV_INPUT_BUFFER_PADDING_SIZE);

    /* Doing payload offset by 16 bytes. Buffer remains the same */
    src->data += 16;
    src->size -= 16;

    dst = av_packet_clone(src);

    assert(dst->size == src->size);
    assert(dst->data == src->data); /* This fails */
}

[gjdfgh]

The fix is just to change

Still a patch would be nice.

[mrlika]

Patch thread:
​http://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194194.html