| Message ID | 20200327125747.13460-8-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/14] mpeg4videodec: do not copy a range of fields at once | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 3/27/2020 9:57 AM, Anton Khirnov wrote: > --- > libavcodec/decode.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 8925ce6edc..f43dc0dd5d 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -1595,7 +1595,7 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) > // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2 > ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w); > if (ret < 0) > - return ret; > + goto fail; > // increase alignment of w for next try (rhs gives the lowest bit set in w) > w += w & ~(w - 1); > > @@ -1606,8 +1606,10 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) > > tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h, > NULL, linesize); > - if (tmpsize < 0) > - return tmpsize; > + if (tmpsize < 0) { > + ret = tmpsize; > + goto fail; > + } > > for (i = 0; i < 3 && data[i + 1]; i++) > size[i] = data[i + 1] - data[i]; Nit: Not really a leak since the pool was not changed in any way if either of these two checks fail, so new calls to get_buffer2() or closing the context would free everything just fine. LGTM with or without changing the commit message to say something about cleaning on error rather than fixing a leak.
Quoting James Almer (2020-03-27 16:03:46) > On 3/27/2020 9:57 AM, Anton Khirnov wrote: > > --- > > libavcodec/decode.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > > index 8925ce6edc..f43dc0dd5d 100644 > > --- a/libavcodec/decode.c > > +++ b/libavcodec/decode.c > > @@ -1595,7 +1595,7 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) > > // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2 > > ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w); > > if (ret < 0) > > - return ret; > > + goto fail; > > // increase alignment of w for next try (rhs gives the lowest bit set in w) > > w += w & ~(w - 1); > > > > @@ -1606,8 +1606,10 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) > > > > tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h, > > NULL, linesize); > > - if (tmpsize < 0) > > - return tmpsize; > > + if (tmpsize < 0) { > > + ret = tmpsize; > > + goto fail; > > + } > > > > for (i = 0; i < 3 && data[i + 1]; i++) > > size[i] = data[i + 1] - data[i]; > > Nit: Not really a leak since the pool was not changed in any way if > either of these two checks fail, so new calls to get_buffer2() or > closing the context would free everything just fine. > > LGTM with or without changing the commit message to say something about > cleaning on error rather than fixing a leak. Ah right, it actually becomes a leak with the previous patch. So maybe I should just merge the two
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 8925ce6edc..f43dc0dd5d 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1595,7 +1595,7 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2 ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w); if (ret < 0) - return ret; + goto fail; // increase alignment of w for next try (rhs gives the lowest bit set in w) w += w & ~(w - 1); @@ -1606,8 +1606,10 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h, NULL, linesize); - if (tmpsize < 0) - return tmpsize; + if (tmpsize < 0) { + ret = tmpsize; + goto fail; + } for (i = 0; i < 3 && data[i + 1]; i++) size[i] = data[i + 1] - data[i];