Opened 3 years ago

Last modified 3 years ago

#5027 new defect

avcodec_free_context frees extradata

Reported by: jyavenard Owned by:
Priority: normal Component: avcodec
Version: unspecified Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

According to the AVCodecContext specification:

AVCodecContext::extradata is "decoding: Set/allocated/freed by user. "

only for encoding is extradata managed by avcodec.

And avcodec_close does the right thing, and only free extradata field if the AVCodecContext instantiated an encoder.

However, avcodec_free_context always free extradata if set:

void avcodec_free_context(AVCodecContext **pavctx)
{
    AVCodecContext *avctx = *pavctx;

    if (!avctx)
        return;

    avcodec_close(avctx);

    av_freep(&avctx->extradata);
    av_freep(&avctx->subtitle_header);
    av_freep(&avctx->intra_matrix);
    av_freep(&avctx->inter_matrix);
    av_freep(&avctx->rc_override);

    av_freep(pavctx);
}

This is incorrect, it assumes that the ownership of extradata is by avcodec for either the decoder or the encoder.

Now the problem has been there for quite a while and it may be too late.

As such, either avcodec_free_context should be made obsolete and replaced with avcodec_free_context2 which would do the right thing.

Or the documentation needs to be updated, and the ownership of extradata now being by avcodec, this assumes that avcodec_close would also free extradata.

Change History (4)

comment:1 Changed 3 years ago by cehoyos

Wouldn't the change you suggest (but unfortunately didn't send to the development mailing list) introduce a memleak?
Could you explain your usecase?

Last edited 3 years ago by cehoyos (previous) (diff)

comment:2 follow-up: Changed 3 years ago by squelart

Since the code has been there for a while, I think the best first move would be to update the documentation to at least match the current implementation, i.e.: 'extradata' (when decoding) is owned by the user *except* when using avcodec_free_context; the user should reset the pointer beforehand if they do not want their extradata to be destroyed.

But I agree with jyavenard that a replacement API avcodec_free_context2 that respects the external ownership of extradata would be more consistent with the documentation and other functions.

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

Replying to squelart:

But I agree with jyavenard that a replacement API avcodec_free_context2 that respects the external ownership of extradata would be more consistent with the documentation and other functions.

Wouldn't this new function avcodec_free_context2() always lead to a memleak? What would be its usecase?

comment:4 Changed 3 years ago by jyavenard

Why would it leak?

extradata was *never* supposed to have been freed automatically by avcodec when in decoder mode.
That it is now freed within avcodec_free_context *is* the bug.

This is per the extradata documentation.

The use case, is respecting the API as it's always been: that is the ownership of extradata is by the user, not avcodec.

Particular use case is that extradata is shared information.

Note: See TracTickets for help on using tickets.