On 21/02/17 08:14, wm4 wrote:
> On Mon, 20 Feb 2017 23:29:20 +0000
> Mark Thompson <[email protected]> wrote:
>
>> On 20/02/17 06:52, wm4 wrote:
>>> On Sun, 19 Feb 2017 18:46:42 +0000
>>> Mark Thompson <[email protected]> wrote:
>>>
>>>> Uses the cl_intel_va_api_media_sharing extension, which supports only
>>>> NV12 surfaces and only mapping from QSV to OpenCL.
>>>
>>> No P010?
>>
>> Nope, NV12 only:
>> <https://www.khronos.org/registry/OpenCL/extensions/intel/cl_intel_va_api_media_sharing.txt>.
>>
>> (Has anyone else ever wanted to do it? Beignet didn't immediately work, I
>> had to send them
>> <https://cgit.freedesktop.org/beignet/commit/?id=3c31216a4ef148ba2e6048e30c62a7a7209407cb>.)
>
> Well, you plan to use OpenCL for generic processing, and that wouldn't
> work with 10 bit decoding. Unless you use the vaapi scaler or so to
> convert to nv12 explicitly. (Which vf_scale_vaapi.c apparently can do
> if it's told to.)
>
> But it's nice to see that it can be easily extended, apparently.
Perhaps slightly surprisingly, generic processing kernels are fine with images
of the same data layout but different element sizes (e.g. NV12 and P010). It
works because everything gets mapped to floats in kernels when using the
normalised formats (CL_UNORM_INT8, CL_UNORM_INT16) - all of the integer values
are abstracted away. (This is also why P010 is implemented but YUV420P10 isn't
- the normalisation as if it were a 16-bit element only makes sense when the
value is in the MSBs.)
So, decode in 8-or-10-bit -> opencl processing -> encode in 8-bit doesn't
actually need any special handling at all if you are careful not to do anything
which breaks the abstraction :)
>>>> +static int opencl_map_from_qsv(AVHWFramesContext *dst_fc, AVFrame *dst,
>>>> + const AVFrame *src, int flags)
>>>> +{
>>>> + AVHWFramesContext *src_fc =
>>>> + (AVHWFramesContext*)src->hw_frames_ctx->data;
>>>> + AVOpenCLDeviceContext *dst_dev = dst_fc->device_ctx->hwctx;
>>>> + OpenCLDeviceContext *ctx = dst_fc->device_ctx->internal->priv;
>>>> + QSVtoOpenCLMapping *mapping = NULL;
>>>> + VASurfaceID va_surface;
>>>> + cl_mem_flags cl_flags;
>>>> + cl_event event;
>>>> + cl_int cle;
>>>> + int err, i;
>>>> +
>>>> + if (src->format == AV_PIX_FMT_QSV) {
>>>> + mfxFrameSurface1 *mfx_surface = (mfxFrameSurface1*)src->data[3];
>>>> + va_surface = *(VASurfaceID*)mfx_surface->Data.MemId;
>>>
>>> Again the somewhat duplicated and "unportable" MemId access.
>>
>> This is in a VAAPI-only function, in spite of the name.
>>
>> I think I need to rename some of this stuff to be clearer. This is really
>> Intel proprietary VAAPI only (and usable as QSV-wrapping-VAAPI), and DXVA2
>> will need separate functions.
>>
>> So maybe:
>>
>> HAVE_INTEL_OPENCL_VAAPI -> HAVE_INTEL_BEIGNET_OPENCL_VAAPI
>> HAVE_INTEL_OPENCL_QSV -> HAVE_INTEL_MEDIA_OPENCL_VAAPI
>>
>> then also HAVE_OPENCL_DXVA2 (which is a standard extension, so not INTEL).
>
> Yeah, I was assuming QSV means QSV on all platforms.
>
> At least HAVE_INTEL_MEDIA_OPENCL_VAAPI sounds like an improvement.
Ok:
HAVE_BEIGNET_OPENCL_VAAPI
HAVE_INTEL_MEDIA_OPENCL_VAAPI
HAVE_OPENCL_DXVA2
(There could also be:
HAVE_OPENCL_D3D11VA (another standard extension)
HAVE_OPENCL_CUDA (not standard, but only Nvidia proprietary so no ambiguity))
>>> Can't comment much. The ifdeffery and complexity of hwcontext_opencl
>>> seems a bit worrying, maybe calling for a more structured approach.
>>
>> It could be split across multple files, but there is a lot of shared code so
>> quite a bit of stuff would have to end up in private headers, which didn't
>> seem very nice. Or did you have something else in mind?
>
> No. If you feel like it'd be an improvement to split it up, I'd
> encourage you, otherwise not.
Ok.
Thank you for all your thoughts on this series, they are much appreciated :)
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
