| Message ID | 20240810060642.58115-1-gnattuoc@me.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/videotoolboxenc: always release supported_props | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
> On Aug 10, 2024, at 14:06, gnattu via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > In vtenc_populate_extradata, supported_props should always be released > to avoid memory leak. > > Regression from cd2f8a22e94700c68b1de7968df11e8bebfd315b > > Signed-off-by: gnattu <gnattuoc@me.com> > --- > libavcodec/videotoolboxenc.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c > index 78ef474d7ae..6cc45db4a96 100644 > --- a/libavcodec/videotoolboxenc.c > +++ b/libavcodec/videotoolboxenc.c > @@ -2787,9 +2787,16 @@ pe_cleanup: > > if (status) { > vtenc_reset(vtctx); > - } else if (vtctx->session) { > - CFRelease(vtctx->session); > - vtctx->session = NULL; > + } else { > + if (vtctx->session) { > + CFRelease(vtctx->session); > + vtctx->session = NULL; > + } > + > + if (vtctx->supported_props) { > + CFRelease(vtctx->supported_props); > + vtctx->supported_props = NULL; > + } > } Could you elaborate on how supported_props is leaked? Isn’t it cleaned by vtenc_close -> vtenc_reset? > > vtctx->frame_ct_out = 0; > -- > 2.39.3 (Apple Git-146) > > _______________________________________________ > 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 Aug 15, 2024, at 19:59, Zhao Zhili <quinkblack@foxmail.com> wrote: > > > >> On Aug 10, 2024, at 14:06, gnattu via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: >> >> In vtenc_populate_extradata, supported_props should always be released >> to avoid memory leak. >> >> Regression from cd2f8a22e94700c68b1de7968df11e8bebfd315b >> >> Signed-off-by: gnattu <gnattuoc@me.com> >> --- >> libavcodec/videotoolboxenc.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c >> index 78ef474d7ae..6cc45db4a96 100644 >> --- a/libavcodec/videotoolboxenc.c >> +++ b/libavcodec/videotoolboxenc.c >> @@ -2787,9 +2787,16 @@ pe_cleanup: >> >> if (status) { >> vtenc_reset(vtctx); >> - } else if (vtctx->session) { >> - CFRelease(vtctx->session); >> - vtctx->session = NULL; >> + } else { >> + if (vtctx->session) { >> + CFRelease(vtctx->session); >> + vtctx->session = NULL; >> + } >> + >> + if (vtctx->supported_props) { >> + CFRelease(vtctx->supported_props); >> + vtctx->supported_props = NULL; >> + } >> } > > Could you elaborate on how supported_props is leaked? Isn’t it cleaned by > vtenc_close -> vtenc_reset? In the function `vtenc_populate_extradata`, a call is made to `vtenc_create_encoder` to create a `VTCompressionSession`. During this process, a `supported_props` dictionary is created. However, if the status is 0, only the compression session is released, while `supported_props` remains allocated. Later, in `vtenc_configure_encoder`, `vtenc_create_encoder` is called again after `vtenc_populate_extradata` returns. This call replaces the reference to `supported_props` with a new dictionary, causing the reference to the old one (created in `vtenc_populate_extradata`) to be lost. As a result, when `vtenc_close -> vtenc_reset` is called, it will only release the most recent `supported_props`, leading to a memory leak as all previous allocations are not properly released. > >> >> vtctx->frame_ct_out = 0; >> -- >> 2.39.3 (Apple Git-146) >> >> _______________________________________________ >> 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". > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
> On Aug 15, 2024, at 22:02, Gnattu OC via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > > >> On Aug 15, 2024, at 19:59, Zhao Zhili <quinkblack@foxmail.com> wrote: >> >> >> >>> On Aug 10, 2024, at 14:06, gnattu via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: >>> >>> In vtenc_populate_extradata, supported_props should always be released >>> to avoid memory leak. >>> >>> Regression from cd2f8a22e94700c68b1de7968df11e8bebfd315b >>> >>> Signed-off-by: gnattu <gnattuoc@me.com> >>> --- >>> libavcodec/videotoolboxenc.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c >>> index 78ef474d7ae..6cc45db4a96 100644 >>> --- a/libavcodec/videotoolboxenc.c >>> +++ b/libavcodec/videotoolboxenc.c >>> @@ -2787,9 +2787,16 @@ pe_cleanup: >>> >>> if (status) { >>> vtenc_reset(vtctx); >>> - } else if (vtctx->session) { >>> - CFRelease(vtctx->session); >>> - vtctx->session = NULL; >>> + } else { >>> + if (vtctx->session) { >>> + CFRelease(vtctx->session); >>> + vtctx->session = NULL; >>> + } >>> + >>> + if (vtctx->supported_props) { >>> + CFRelease(vtctx->supported_props); >>> + vtctx->supported_props = NULL; >>> + } >>> } >> >> Could you elaborate on how supported_props is leaked? Isn’t it cleaned by >> vtenc_close -> vtenc_reset? > > In the function `vtenc_populate_extradata`, a call is made to `vtenc_create_encoder` to create a `VTCompressionSession`. During this process, a `supported_props` dictionary is created. However, if the status is 0, only the compression session is released, while `supported_props` remains allocated. > > Later, in `vtenc_configure_encoder`, `vtenc_create_encoder` is called again after `vtenc_populate_extradata` returns. This call replaces the reference to `supported_props` with a new dictionary, causing the reference to the old one (created in `vtenc_populate_extradata`) to be lost. As a result, when `vtenc_close -> vtenc_reset` is called, it will only release the most recent `supported_props`, leading to a memory leak as all previous allocations are not properly released. I see. It’s more appropriate to use a local variable if supported_props is always released at the end of this function. On the other hand, although supported_props is write only variable for now, it’s meant to be used outside of this function is the future. So check and release supported_props should be done just before being overwritten. > >> >>> >>> vtctx->frame_ct_out = 0; >>> -- >>> 2.39.3 (Apple Git-146) >>> >>> _______________________________________________ >>> 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". >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe". > > _______________________________________________ > 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/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c index 78ef474d7ae..6cc45db4a96 100644 --- a/libavcodec/videotoolboxenc.c +++ b/libavcodec/videotoolboxenc.c @@ -2787,9 +2787,16 @@ pe_cleanup: if (status) { vtenc_reset(vtctx); - } else if (vtctx->session) { - CFRelease(vtctx->session); - vtctx->session = NULL; + } else { + if (vtctx->session) { + CFRelease(vtctx->session); + vtctx->session = NULL; + } + + if (vtctx->supported_props) { + CFRelease(vtctx->supported_props); + vtctx->supported_props = NULL; + } } vtctx->frame_ct_out = 0;
In vtenc_populate_extradata, supported_props should always be released to avoid memory leak. Regression from cd2f8a22e94700c68b1de7968df11e8bebfd315b Signed-off-by: gnattu <gnattuoc@me.com> --- libavcodec/videotoolboxenc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)