[FFmpeg-devel] [PATCH] Change how vaapi invalid id are verified

[Gwenole Beauchesne]

Hi,

2013/3/7 LANGLOIS Olivier PIS -EXT <olivier.pis.langlois at transport.alstom.com>:
> This is a theoretical fix has the existing way did not cause problems (except maybe an unoticed leak) but the patch has also been tested successfully with Intel iegd 10.3 driver on a Poulsbo chipset using mpeg4 and H.264 streams.

The patch can be correct in the general case, i.e. when we actually
succeed in creating resources. However, there is a corner case that
could cause issues. Imagine this scenario: FF/VA context is created,
and zero-initialized as documented. A frame is attempted for decoding
but somehow fails in allocating resources. pic param buf id might be
set correctly, but not others. So, the destroy_buffers() will send an
invalid id to the VA driver (0 != VA_INVALID_ID) :).

In practice, IEGD/EMGD indeed allocates VA ids from 0. However, I
think it is unlikely to get a VA surface or buffer id set to 0 because
other ids for eg. context or config id would have been allocated
first. So, the range of surface/buffer ids usually starts around 2
IIRC.

The proper way to fix the FFmpeg issue would be to extend your patch
with the following:
1) Update the vaapi.h comment to mention zero-initialization of the
structure + set all ids to VA_INVALID_ID ; --or?--
2) However, this would mean to fix all user applications as well. So
another thing to do could be to add extra fields at the end that serve
as (internal) markers to know whether ids have been allocated or not.
This is not needed for slice ids since this is an array and we already
have n_slice_buf_ids for that.

e.g. you could place an uintptr_t reserved[2]; at the end of
vaapi_context: 1 for flags, one for something else pointer-based in
the future, though I don't immediately see anything specific yet.

(1) would be the most correct thing to do, but (2) would provide less
annoyance. Anyway, in both cases the user application would need to be
rebuilt but at least with (2) the fix silently gets in and can happen
at the next build bot iteration if any, or simple manual rebuild.

There is a possibility (3): merge in open/close or init/fini hooks for
hwaccel and add a VA-API specific hook. This means an internal API
change and a careful placement of the hwaccel->open() call to make it
happen first after user initialized hwaccel context. The net benefit
is no change needed in the user application ecosystem that uses
FFmpeg/vaapi. But while we are at it, this opens the door to more
interesting changes to make to hwaccel. :)

Regards,
Gwenole.