| Message ID | 20200126064051.22715-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/dvenc: Replace write_trailer by deinit function | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
lgtm On 1/26/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > The old write_trailer only freed memory, so it is better to make a > dedicated deinit function out of it. Given that this function will also > be called when writing the header fails, one can also remove code that > frees already allocated fifos when allocating another one fails. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > There is something strange going on in this muxer: eafa8e85929 increased > several limits for the number of audio streams to four, yet the initial > check in dv_init_mux still makes sure that at most two audio streams > exist. Has updating this (and probably also the comment for n_ast which > still says that there are up to two stereo tracks allowed) simply been > forgotten? > > libavformat/dvenc.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/libavformat/dvenc.c b/libavformat/dvenc.c > index a7d1413eb2..b89ad4d1c8 100644 > --- a/libavformat/dvenc.c > +++ b/libavformat/dvenc.c > @@ -364,10 +364,6 @@ static DVMuxContext* dv_init_mux(AVFormatContext* s) > > for (i=0; i < c->n_ast; i++) { > if (c->ast[i] && !(c->audio_data[i]=av_fifo_alloc_array(100, > MAX_AUDIO_FRAME_SIZE))) { > - while (i > 0) { > - i--; > - av_fifo_freep(&c->audio_data[i]); > - } > goto bail_out; > } > } > @@ -378,13 +374,6 @@ bail_out: > return NULL; > } > > -static void dv_delete_mux(DVMuxContext *c) > -{ > - int i; > - for (i=0; i < c->n_ast; i++) > - av_fifo_freep(&c->audio_data[i]); > -} > - > static int dv_write_header(AVFormatContext *s) > { > AVRational rate; > @@ -432,10 +421,12 @@ static int dv_write_packet(struct AVFormatContext *s, > AVPacket *pkt) > * Currently we simply drop the last frame. I don't know whether this > * is the best strategy of all > */ > -static int dv_write_trailer(struct AVFormatContext *s) > +static void dv_deinit(AVFormatContext *s) > { > - dv_delete_mux(s->priv_data); > - return 0; > + DVMuxContext *c = s->priv_data; > + > + for (int i = 0; i < c->n_ast; i++) > + av_fifo_freep(&c->audio_data[i]); > } > > AVOutputFormat ff_dv_muxer = { > @@ -447,5 +438,5 @@ AVOutputFormat ff_dv_muxer = { > .video_codec = AV_CODEC_ID_DVVIDEO, > .write_header = dv_write_header, > .write_packet = dv_write_packet, > - .write_trailer = dv_write_trailer, > + .deinit = dv_deinit, > }; > -- > 2.20.1 > > _______________________________________________ > 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".
On Sun, Jan 26, 2020 at 10:12:16AM +0100, Paul B Mahol wrote:
> lgtm
will apply
thx
[...]
diff --git a/libavformat/dvenc.c b/libavformat/dvenc.c index a7d1413eb2..b89ad4d1c8 100644 --- a/libavformat/dvenc.c +++ b/libavformat/dvenc.c @@ -364,10 +364,6 @@ static DVMuxContext* dv_init_mux(AVFormatContext* s) for (i=0; i < c->n_ast; i++) { if (c->ast[i] && !(c->audio_data[i]=av_fifo_alloc_array(100, MAX_AUDIO_FRAME_SIZE))) { - while (i > 0) { - i--; - av_fifo_freep(&c->audio_data[i]); - } goto bail_out; } } @@ -378,13 +374,6 @@ bail_out: return NULL; } -static void dv_delete_mux(DVMuxContext *c) -{ - int i; - for (i=0; i < c->n_ast; i++) - av_fifo_freep(&c->audio_data[i]); -} - static int dv_write_header(AVFormatContext *s) { AVRational rate; @@ -432,10 +421,12 @@ static int dv_write_packet(struct AVFormatContext *s, AVPacket *pkt) * Currently we simply drop the last frame. I don't know whether this * is the best strategy of all */ -static int dv_write_trailer(struct AVFormatContext *s) +static void dv_deinit(AVFormatContext *s) { - dv_delete_mux(s->priv_data); - return 0; + DVMuxContext *c = s->priv_data; + + for (int i = 0; i < c->n_ast; i++) + av_fifo_freep(&c->audio_data[i]); } AVOutputFormat ff_dv_muxer = { @@ -447,5 +438,5 @@ AVOutputFormat ff_dv_muxer = { .video_codec = AV_CODEC_ID_DVVIDEO, .write_header = dv_write_header, .write_packet = dv_write_packet, - .write_trailer = dv_write_trailer, + .deinit = dv_deinit, };
The old write_trailer only freed memory, so it is better to make a dedicated deinit function out of it. Given that this function will also be called when writing the header fails, one can also remove code that frees already allocated fifos when allocating another one fails. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- There is something strange going on in this muxer: eafa8e85929 increased several limits for the number of audio streams to four, yet the initial check in dv_init_mux still makes sure that at most two audio streams exist. Has updating this (and probably also the comment for n_ast which still says that there are up to two stereo tracks allowed) simply been forgotten? libavformat/dvenc.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)