Quoting Diego Biurrun (2016-02-16 15:36:05)
> On Tue, Feb 16, 2016 at 12:16:09PM +0000, Mark Thompson wrote:
> > On 16/02/16 11:38, Diego Biurrun wrote:
> > > On Mon, Feb 15, 2016 at 11:24:31PM +0000, Mark Thompson wrote:
> > >> + fail:
> > >> + if (ctx->format_list)
> > >> + av_freep(&ctx->format_list);
> > >> + if (image_list)
> > >> + av_free(image_list);
> > >> + if (attr_list)
> > >> + av_free(attr_list);
> > >
> > > The NULL checks before calling av_free() are unnecessary.
> >
> > Hmm, true. I would prefer to leave them to clarify when the deallocation
> > has to
> > happen (if something more complex than just free() needs to be done, say),
> > but I
> > can remove them if you feel strongly about it.
>
> Please remove them, we removed similar NULL checks everywhere a while ago,
> let's not undo that work.
>
> > >> +static void vaapi_buffer_free(void *opaque, uint8_t *data)
> > >> +{
> > >> + AVHWFramesContext *hwfc = opaque;
> > >> + AVVAAPIDeviceContext *hwctx = hwfc->device_ctx->hwctx;
> > >> + VASurfaceID surface_id;
> > >> + VAStatus vas;
> > >> +
> > >> + surface_id = (VASurfaceID)(uintptr_t)data;
> > >
> > > Gah, why the double cast?
> >
> > Casting a pointer to a narrower integer type is dubious in C (can be
> > undefined
> > behaviour, even on unsigned integers). It shouldn't directly matter here
> > because it originally came from the target type, but gcc does warn
> > (VASurfaceID
> > is unsigned int, so narrower given 64-bit pointers) and I'd prefer not to
> > have
> > the warning.
>
> This seems deeply suspicious, how does it even work at all?
Why wouldn't it work? The value was originally a VASurfaceID, it just
gets passed around as an uint8_t* pointer.
Also, exactly the same casts are used in other places in hwaccel code.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
