| Message ID | 20210204191005.48190-17-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | deprecate av_init_packet() and sizeof(AVPacket) as part of the ABI | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
James Almer: > Signed-off-by: James Almer <jamrial@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). > + } > + > 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. > 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", > }; >
On 2/7/2021 4:47 AM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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); + } + 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); 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", };
Signed-off-by: James Almer <jamrial@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(-)