Opened 3 years ago

Closed 3 years ago

#5543 closed defect (fixed)

av_packet_ref incorrectly processes packets with offset between data and buffer

Reported by: mrlika Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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.

Change History (9)

comment:1 follow-up: Changed 3 years ago by cehoyos

  • Priority changed from critical to normal

How can I reproduce the issue?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by 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.

comment:3 in reply to: ↑ 2 Changed 3 years ago by cehoyos

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?

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by 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.

comment:6 Changed 3 years ago by 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 */
}

comment:7 Changed 3 years ago by gjdfgh

The fix is just to change

Still a patch would be nice.

comment:9 Changed 3 years ago by mrlika

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.