Re: [FFmpeg-devel] [PATCH 1/2] vf_paletteuse: Add error checking to apply_palette
On 1/2/2018 10:16 PM, Clément Bœsch wrote: > I don't think you'll be much off by always assuming ENOMEM here when > getting a NULL out frame, I think that's the common idiom when a function > supposed to return allocated stuff returns NULL. > > But that's not very important, feel free to push as is if you prefer that > way. I'll change it to ENOMEM before push, then. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] vf_paletteuse: Add error checking to apply_palette
On Tue, Jan 02, 2018 at 10:02:25PM +, Derek Buitenhuis wrote: > On 1/2/2018 9:52 PM, Clément Bœsch wrote: > > not exactly sure why you haven't just checked if `out` wasn't NULL, but it > > should be fine that way too if you prefer it. > > > > I guess that's only to provide a finer grain error handling? It would make > > sense if ff_get_video_buffer was returning a meaningful error, but so far > > you're just assuming EINVAL when it could be ENOMEM, so I don't really get > > it. > > s->set_frame can return ENOMEM, which is why I made it finer grained. > > I'm not really sure what to return for ff_get_video_buffer failure, since > it wasn't designed with any error mechanism for some reason. > I don't think you'll be much off by always assuming ENOMEM here when getting a NULL out frame, I think that's the common idiom when a function supposed to return allocated stuff returns NULL. But that's not very important, feel free to push as is if you prefer that way. -- Clément B. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] vf_paletteuse: Add error checking to apply_palette
On 1/2/2018 9:52 PM, Clément Bœsch wrote: > not exactly sure why you haven't just checked if `out` wasn't NULL, but it > should be fine that way too if you prefer it. > > I guess that's only to provide a finer grain error handling? It would make > sense if ff_get_video_buffer was returning a meaningful error, but so far > you're just assuming EINVAL when it could be ENOMEM, so I don't really get > it. s->set_frame can return ENOMEM, which is why I made it finer grained. I'm not really sure what to return for ff_get_video_buffer failure, since it wasn't designed with any error mechanism for some reason. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] vf_paletteuse: Add error checking to apply_palette
On Mon, Jan 01, 2018 at 11:28:36AM -0500, Derek Buitenhuis wrote:
> This fixes a segfault caused by passing NULL to ff_filter_frame
> when an error occurs.
>
> Signed-off-by: Derek Buitenhuis
> ---
> libavfilter/vf_paletteuse.c | 25 -
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
> index 1980907..ede2e2e 100644
> --- a/libavfilter/vf_paletteuse.c
> +++ b/libavfilter/vf_paletteuse.c
> @@ -894,9 +894,9 @@ static void set_processing_window(enum diff_mode
> diff_mode,
> *hp = height;
> }
>
> -static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in)
> +static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
> {
> -int x, y, w, h;
> +int x, y, w, h, ret;
> AVFilterContext *ctx = inlink->dst;
> PaletteUseContext *s = ctx->priv;
> AVFilterLink *outlink = inlink->dst->outputs[0];
> @@ -904,7 +904,8 @@ static AVFrame *apply_palette(AVFilterLink *inlink,
> AVFrame *in)
> AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> if (!out) {
> av_frame_free(&in);
> -return NULL;
> +*outf = NULL;
> +return AVERROR(EINVAL);
> }
> av_frame_copy_props(out, in);
>
> @@ -918,21 +919,25 @@ static AVFrame *apply_palette(AVFilterLink *inlink,
> AVFrame *in)
> av_frame_make_writable(s->last_in) < 0) {
> av_frame_free(&in);
> av_frame_free(&out);
> -return NULL;
> +*outf = NULL;
> +return AVERROR(EINVAL);
> }
>
> ff_dlog(ctx, "%dx%d rect: (%d;%d) -> (%d,%d) [area:%dx%d]\n",
> w, h, x, y, x+w, y+h, in->width, in->height);
>
> -if (s->set_frame(s, out, in, x, y, w, h) < 0) {
> +ret = s->set_frame(s, out, in, x, y, w, h);
> +if (ret < 0) {
> av_frame_free(&out);
> -return NULL;
> +*outf = NULL;
> +return ret;
> }
> memcpy(out->data[1], s->palette, AVPALETTE_SIZE);
> if (s->calc_mean_err)
> debug_mean_error(s, in, out, inlink->frame_count_out);
> av_frame_free(&in);
> -return out;
> +*outf = out;
> +return 0;
> }
>
> static int config_output(AVFilterLink *outlink)
> @@ -1011,7 +1016,7 @@ static int load_apply_palette(FFFrameSync *fs)
> AVFilterContext *ctx = fs->parent;
> AVFilterLink *inlink = ctx->inputs[0];
> PaletteUseContext *s = ctx->priv;
> -AVFrame *master, *second, *out;
> +AVFrame *master, *second, *out = NULL;
> int ret;
>
> // writable for error diffusal dithering
> @@ -1025,7 +1030,9 @@ static int load_apply_palette(FFFrameSync *fs)
> if (!s->palette_loaded) {
> load_palette(s, second);
> }
> -out = apply_palette(inlink, master);
> +ret = apply_palette(inlink, master, &out);
> +if (ret < 0)
> +goto error;
> return ff_filter_frame(ctx->outputs[0], out);
>
not exactly sure why you haven't just checked if `out` wasn't NULL, but it
should be fine that way too if you prefer it.
I guess that's only to provide a finer grain error handling? It would make
sense if ff_get_video_buffer was returning a meaningful error, but so far
you're just assuming EINVAL when it could be ENOMEM, so I don't really get
it.
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] vf_paletteuse: Add error checking to apply_palette
This fixes a segfault caused by passing NULL to ff_filter_frame
when an error occurs.
Signed-off-by: Derek Buitenhuis
---
libavfilter/vf_paletteuse.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
index 1980907..ede2e2e 100644
--- a/libavfilter/vf_paletteuse.c
+++ b/libavfilter/vf_paletteuse.c
@@ -894,9 +894,9 @@ static void set_processing_window(enum diff_mode diff_mode,
*hp = height;
}
-static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in)
+static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
{
-int x, y, w, h;
+int x, y, w, h, ret;
AVFilterContext *ctx = inlink->dst;
PaletteUseContext *s = ctx->priv;
AVFilterLink *outlink = inlink->dst->outputs[0];
@@ -904,7 +904,8 @@ static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame
*in)
AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
if (!out) {
av_frame_free(&in);
-return NULL;
+*outf = NULL;
+return AVERROR(EINVAL);
}
av_frame_copy_props(out, in);
@@ -918,21 +919,25 @@ static AVFrame *apply_palette(AVFilterLink *inlink,
AVFrame *in)
av_frame_make_writable(s->last_in) < 0) {
av_frame_free(&in);
av_frame_free(&out);
-return NULL;
+*outf = NULL;
+return AVERROR(EINVAL);
}
ff_dlog(ctx, "%dx%d rect: (%d;%d) -> (%d,%d) [area:%dx%d]\n",
w, h, x, y, x+w, y+h, in->width, in->height);
-if (s->set_frame(s, out, in, x, y, w, h) < 0) {
+ret = s->set_frame(s, out, in, x, y, w, h);
+if (ret < 0) {
av_frame_free(&out);
-return NULL;
+*outf = NULL;
+return ret;
}
memcpy(out->data[1], s->palette, AVPALETTE_SIZE);
if (s->calc_mean_err)
debug_mean_error(s, in, out, inlink->frame_count_out);
av_frame_free(&in);
-return out;
+*outf = out;
+return 0;
}
static int config_output(AVFilterLink *outlink)
@@ -1011,7 +1016,7 @@ static int load_apply_palette(FFFrameSync *fs)
AVFilterContext *ctx = fs->parent;
AVFilterLink *inlink = ctx->inputs[0];
PaletteUseContext *s = ctx->priv;
-AVFrame *master, *second, *out;
+AVFrame *master, *second, *out = NULL;
int ret;
// writable for error diffusal dithering
@@ -1025,7 +1030,9 @@ static int load_apply_palette(FFFrameSync *fs)
if (!s->palette_loaded) {
load_palette(s, second);
}
-out = apply_palette(inlink, master);
+ret = apply_palette(inlink, master, &out);
+if (ret < 0)
+goto error;
return ff_filter_frame(ctx->outputs[0], out);
error:
--
1.8.3.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
