[FFmpeg-devel] [PATCH 16/50] avformat/dv: use av_packet_alloc() to allocate packets
James Almer
jamrial at gmail.com
Sun Feb 7 15:14:51 EET 2021
On 2/7/2021 4:47 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> This one must be committed after the upcoming major bump, as it changes
>> the return value of an avpriv function.
>>
>> libavdevice/iec61883.c | 2 +-
>> libavformat/dv.c | 56 ++++++++++++++++++++++++++++--------------
>> 2 files changed, 38 insertions(+), 20 deletions(-)
>>
>> diff --git a/libavdevice/iec61883.c b/libavdevice/iec61883.c
>> index cafafb2672..4582ada151 100644
>> --- a/libavdevice/iec61883.c
>> +++ b/libavdevice/iec61883.c
>> @@ -191,7 +191,7 @@ static int iec61883_parse_queue_dv(struct iec61883_data *dv, AVPacket *pkt)
>> int size;
>>
>> size = avpriv_dv_get_packet(dv->dv_demux, pkt);
>> - if (size > 0)
>> + if (size)
>> return size;
>>
>> packet = dv->queue_first;
>> diff --git a/libavformat/dv.c b/libavformat/dv.c
>> index 26a78139f5..f5c0c44afc 100644
>> --- a/libavformat/dv.c
>> +++ b/libavformat/dv.c
>> @@ -45,7 +45,7 @@ struct DVDemuxContext {
>> AVFormatContext* fctx;
>> AVStream* vst;
>> AVStream* ast[4];
>> - AVPacket audio_pkt[4];
>> + AVPacket *audio_pkt[4];
>> uint8_t audio_buf[4][8192];
>> int ach;
>> int frames;
>> @@ -261,11 +261,10 @@ static int dv_extract_audio_info(DVDemuxContext *c, const uint8_t *frame)
>> c->ast[i]->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>> c->ast[i]->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE;
>>
>> - av_init_packet(&c->audio_pkt[i]);
>> - c->audio_pkt[i].size = 0;
>> - c->audio_pkt[i].data = c->audio_buf[i];
>> - c->audio_pkt[i].stream_index = c->ast[i]->index;
>> - c->audio_pkt[i].flags |= AV_PKT_FLAG_KEY;
>> + c->audio_pkt[i]->size = 0;
>> + c->audio_pkt[i]->data = c->audio_buf[i];
>> + c->audio_pkt[i]->stream_index = c->ast[i]->index;
>> + c->audio_pkt[i]->flags |= AV_PKT_FLAG_KEY;
>> }
>> c->ast[i]->codecpar->sample_rate = dv_audio_frequency[freq];
>> c->ast[i]->codecpar->channels = 2;
>> @@ -332,6 +331,12 @@ static int dv_init_demux(AVFormatContext *s, DVDemuxContext *c)
>> c->vst->codecpar->bit_rate = 25000000;
>> c->vst->start_time = 0;
>>
>> + for (int i = 0; i < 4; i++) {
>> + c->audio_pkt[i] = av_packet_alloc();
>> + if (!c->audio_pkt[i])
>> + return AVERROR(ENOMEM);
>
> This leads to lots of leaks on error, for both avpriv_dv_init_demux (for
> which a free function will be necessary) as well as the dv demuxer
> (whose read_close function is not called automatically on error).
Will fix.
>
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -353,13 +358,15 @@ DVDemuxContext *avpriv_dv_init_demux(AVFormatContext *s)
>>
>> int avpriv_dv_get_packet(DVDemuxContext *c, AVPacket *pkt)
>> {
>> - int size = -1;
>> + int size = 0;
>> int i;
>>
>> for (i = 0; i < c->ach; i++) {
>> - if (c->ast[i] && c->audio_pkt[i].size) {
>> - *pkt = c->audio_pkt[i];
>> - c->audio_pkt[i].size = 0;
>> + if (c->ast[i] && c->audio_pkt[i]->size) {
>> + int ret = av_packet_ref(pkt, c->audio_pkt[i]);
>> + if (ret < 0)
>> + return ret;
>> + c->audio_pkt[i]->size = 0;
>> size = pkt->size;
>> break;
>> }
>> @@ -384,9 +391,9 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
>> /* FIXME: in case of no audio/bad audio we have to do something */
>> size = dv_extract_audio_info(c, buf);
>> for (i = 0; i < c->ach; i++) {
>> - c->audio_pkt[i].pos = pos;
>> - c->audio_pkt[i].size = size;
>> - c->audio_pkt[i].pts = (c->sys->height == 720) ? (c->frames & ~1) : c->frames;
>> + c->audio_pkt[i]->pos = pos;
>> + c->audio_pkt[i]->size = size;
>> + c->audio_pkt[i]->pts = (c->sys->height == 720) ? (c->frames & ~1) : c->frames;
>> ppcm[i] = c->audio_buf[i];
>> }
>> if (c->ach)
>> @@ -396,15 +403,14 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
>> * channels 0,1 and odd 2,3. */
>> if (c->sys->height == 720) {
>> if (buf[1] & 0x0C) {
>> - c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
>> + c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
>> } else {
>> - c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
>> + c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
>> }
>> }
>>
>> /* Now it's time to return video packet */
>> size = dv_extract_video_info(c, buf);
>> - av_init_packet(pkt);
>
> This function here is a bit nuts: It predates reference-counted buffers
> and so it simply resets pkt->buf (it in fact treats pkt purely as
> something for output as if it were already blank). The mov and avi
> demuxers work around this by copying pkt->buf and putting it in the
> returned packet afterwards (which is probably unnecessary when the above
> call is ; whereas the iec61883 device/demuxer really puts a blank packet
> in there and makes it refcounted afterwards. It would be nice if this
> whole mess were to be cleaned up.
I agree it could be better, but I'm not going to rewrite a function i
can't test. Last time i handled this shared code to fix a massive memory
leak i had the help of a trac user that could test the iec61883 device.
>
>> pkt->data = buf;
>> pkt->pos = pos;
>> pkt->size = size;
>> @@ -439,8 +445,8 @@ static int64_t dv_frame_offset(AVFormatContext *s, DVDemuxContext *c,
>> void ff_dv_offset_reset(DVDemuxContext *c, int64_t frame_offset)
>> {
>> c->frames = frame_offset;
>> - c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
>> - c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
>> + c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
>> + c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
>> }
>>
>> /************************************************************
>> @@ -539,7 +545,7 @@ static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
>>
>> size = avpriv_dv_get_packet(&c->dv_demux, pkt);
>>
>> - if (size < 0) {
>> + if (!size) {
>> int ret;
>> int64_t pos = avio_tell(s->pb);
>> if (!c->dv_demux.sys)
>> @@ -614,6 +620,17 @@ static int dv_probe(const AVProbeData *p)
>> return 0;
>> }
>>
>> +static int dv_read_close(AVFormatContext *s)
>> +{
>> + RawDVContext *r = s->priv_data;
>> + DVDemuxContext *c = &r->dv_demux;
>> +
>> + for (int i = 0; i < 4; i++)
>> + av_packet_free(&c->audio_pkt[i]);
>> +
>> + return 0;
>> +}
>> +
>> AVInputFormat ff_dv_demuxer = {
>> .name = "dv",
>> .long_name = NULL_IF_CONFIG_SMALL("DV (Digital Video)"),
>> @@ -622,5 +639,6 @@ AVInputFormat ff_dv_demuxer = {
>> .read_header = dv_read_header,
>> .read_packet = dv_read_packet,
>> .read_seek = dv_read_seek,
>> + .read_close = dv_read_close,
>> .extensions = "dv,dif",
>> };
>>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list