diff mbox

[FFmpeg-devel,2/2] vf_paletteuse: Don't free the second frame from ff_framesync_dualinput_get_writable on error

Message ID 1514824117-15587-3-git-send-email-derek.buitenhuis@gmail.com
State Accepted
Commit 631fa0432be8968e0fd372595749b918224946df
Headers show

Commit Message

Derek Buitenhuis Jan. 1, 2018, 4:28 p.m. UTC
This fixes a double free in he error case.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
This does fix the double free, but I am unsure if it is the correct free
to removed to fix it. Comments welcome.
---
 libavfilter/vf_paletteuse.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Nicolas George Jan. 2, 2018, 1:46 p.m. UTC | #1
Derek Buitenhuis (2018-01-01):
> This fixes a double free in he error case.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> This does fix the double free, but I am unsure if it is the correct free
> to removed to fix it. Comments welcome.
> ---
>  libavfilter/vf_paletteuse.c | 1 -
>  1 file changed, 1 deletion(-)

I confirm it is correct with regard to the framesync API. But I do not
maintain vf_paletteuse.

Regards,
Clément Bœsch Jan. 2, 2018, 9:53 p.m. UTC | #2
On Mon, Jan 01, 2018 at 11:28:37AM -0500, Derek Buitenhuis wrote:
> This fixes a double free in he error case.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> This does fix the double free, but I am unsure if it is the correct free
> to removed to fix it. Comments welcome.
> ---
>  libavfilter/vf_paletteuse.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
> index ede2e2e..c2d0c6b 100644
> --- a/libavfilter/vf_paletteuse.c
> +++ b/libavfilter/vf_paletteuse.c
> @@ -1037,7 +1037,6 @@ static int load_apply_palette(FFFrameSync *fs)
>  
>  error:
>      av_frame_free(&master);
> -    av_frame_free(&second);
>      return ret;
>  }

That's some weird ownership semantic for the error-path, but Nicolas knows
better this API so I'll trust him on this one.
Derek Buitenhuis Jan. 2, 2018, 10 p.m. UTC | #3
On 1/2/2018 9:53 PM, Clément Bœsch wrote:
> That's some weird ownership semantic for the error-path, but Nicolas knows
> better this API so I'll trust him on this one.

Yeah it was weird for me to, but I looked into its implementation to find out.

- Derek
Nicolas George Jan. 3, 2018, 3:28 p.m. UTC | #4
Clement Boesch (2018-01-02):
> That's some weird ownership semantic for the error-path, but Nicolas knows
> better this API so I'll trust him on this one.

I agree it is not the most obvious behaviour, but it is the most
convenient with the logic of dualinput (which you introduced, IIRC,
based on the workings of overlay): the first input is considered the
main input, the frames on it are the frames that are actually filtered,
while the second input only provides read-only reference material.

If you have a look at the documentation patch I sent yesterday, I can
push soon.

Regards,
diff mbox

Patch

diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
index ede2e2e..c2d0c6b 100644
--- a/libavfilter/vf_paletteuse.c
+++ b/libavfilter/vf_paletteuse.c
@@ -1037,7 +1037,6 @@  static int load_apply_palette(FFFrameSync *fs)
 
 error:
     av_frame_free(&master);
-    av_frame_free(&second);
     return ret;
 }