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?
> >> + ref = av_buffer_create((uint8_t*)(uintptr_t)surface_id,
> >> + sizeof(surface_id), &vaapi_buffer_free,
> >> + hwfc, AV_BUFFER_FLAG_READONLY);
> >
> > same question about the cast
>
> While this one is ok, it feels better to be exactly consistent with the cast
> in
> the other direction.
It seems similarly suspicious to me ..
> Thanks for the review :)
You are most welcome.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
