Fix the workaround code in VA-API External frame allocator

[sreerenjb]

I would like to get some clarification on ExternalFrameAllocator code which is not documented.

The code in vaapi_allocator.cpp (MediaSamples/sample/sample_common/src) seems really confusing:

The AllocImpl() code is allocating buffers of type VACodedBufferSegment (which is supposed to be only using for storing Encoded bitstream), and then assigning it to VASurfaces (which is not supposed to be typecast like this IMHO), Then the lock/unlock() routines simply assigning the codedBufferSEgment to the mfxFrameData->Y (mfxFrameData's Y should be pointing to the raw luminance array). Is this implementation is a hack to make use of preallocated buffer pool for EncodedBitStream (Seems like only h265 is utilizing it)? If so we need to do something better IMHO!

[dmitryermilov]

Hi Sree,

This code of allocation of vaBuffer's in external allocator is not actual anymore. In the past when application set an an external allocator then all allocation requests for vaSurface's and vaBuffer's (with type == vaEncCodedBufferType) went thru the external allocator. Now allocation of vaBuffers (vaEncCodedBufferType) happens via internal memory allocator. You may find it in _studio/shared/src/libmfx_allocator_vaapi.cpp.

Is this implementation is a hack to make use of preallocated buffer pool for EncodedBitStream .

Yes, the purpose was to allow MediaSDK encoder implementations to operate with VA surfaces and VA buffers in the same way because actually they share the same interfaces (create/destroy, lock/unlock).

(Seems like only h265 is utilizing it)?

All encoders.

Regards, Dmitry

[sreerenjb]

@dmitryermilov Thanks for the clarification. Happy to hear that you guys removed the hack because it was so awkward to see the allocate the non-raw buffer and assigning it to the raw data pointer in all middlewares.

According to my testing, this hack is needed even in MediaSamplesLinux_2017R3_b698 in order to make the HEVC encode working. Rest of the encoders doesn't have any issue.

"This code of allocation of vaBuffer's in external allocator is not actual anymore. " => I guess it is true only for the Open source version of Msdk, right?

[dmitryermilov]

It is not a hack. The vaapi allocator allocates objects in video memory - surfaces and buffers . Assigning to Y pointer is a way for encoders to operate with video memory objects via the same interface. I'll take a look at HEVCe code path.

Regards, Dmitry

[sreerenjb]

The code is using MFX_FOURCC_P8 for checking whether it is expecting a coded buffer where the format MFX_FOURCC_P8 is D3DFMT_P8 and using a raw format forucc like this is a "HACK" which usually never get accepted in open source projects. The P208 is the fourcc code for 8bit planar 4:2:2 format, what if the driver supports P208 as input format in future? Then we will end up adding the "hack on top of hack" like: If (MSDK VERSION == XXX) P208 is not a yuv format, Sorry. Else Ya, we consider this a YUV,

See the similar one in ffmpeg/libav: https://patches.libav.org/patch/62401/

Also hacks like this should be mentioned in the specification..

[dmitryermilov]

MFX_FOURCC_P8 is an SDK internal color format which represents bistream buffer. Please refer to MediaSDK manual. Mapping it to VA_FOURCC_P208 in the allocator is actually a bug, probably will be fixed soon.

Regards, Dmitry

[sreerenjb]

What I can see in MSDK source code is this: case MFX_FOURCC_P8: return D3DFMT_P8; According to D3D spec, D3DFMT_P8 is "8 bit color indexed" format.

Yup, usage of "VA_FOURCC_P208" is more serious since it is clearly a RAW format defined in VA spec.

Also assigning a "CodedBufferSegment memory pointer" to "VASurfaceID" is a hack. I understood it has been used for internal MSDK purpose. But now MediaSDK is an open source project and we the opensource middleware developers started looking to the code+spec for proper integration with our frameworks. And it is difficult for us to add code like this for specific versions of MSDK.

[lu-zero]

Having that part clarified and/or possibly replaced with something less unexpected would be a boon indeed (the patch referred and the follow up is still pending since it had deemed too brittle/problematic to stay in the vaapi hwaccel code)

[artem-shaporenko]

It is an issue, for HEVC it is a problem(and other encoder plugins using HW acceleration), as it is plugin which doesn't has proper access to internal allocator, that is why it is going to external one. Dima, I'd suggest to look at external allocator code and fix it, later we will move HEVC encoder to be part of libary and this issue will be fixed, external allocator code for bitstream buffer will be remoived.

[dmitryermilov]

I agree that it should be fixed. The only concern is whether the scenario when vaBuffer is allocated by external allocator and accessed by internal one is possible. If so, when we have a problem. What do you think?

[lizhong1008]

"Moving HEVC encoder to be part of libary" is a good idea since it can make hevc encoding can have some implementation to get coded buffer as h264 encoding. I am wonder when this it will be done then close it issue? One concern is that it will also require to check MSDK version and remove the existing loading HEVC hw plugin code in ffmpeg/gstreamer.

[artem-shaporenko]

Work already started, decoders(HEVC, VP9, VP8) already moved, just waiting to be merged to github. Encoders on the way implementation. I expect to be ready some time near august, but can't say exact date now.

[lizhong1008]

@artem-shaporenko , thanks for update. Glad to know that.

[artem-shaporenko]

I agree that it should be fixed. The only concern is whether the scenario when vaBuffer is allocated by external allocator and accessed by internal one is possible. If so, when we have a problem. What do you think? Dima, there are no situations for allocators, but situation when wrongly implemented component can access resources wrongly is pretty usual.

[fzhar]

we're ready change this in samples allocator as soon as required changes are made in library.

[dmitryermilov]

@fzhar , it's not clear to me. What do you think should exactly be changed in the library ?

[lizhong1008]

What is the latest status of this issue? Will be appreciated if you can give any update.

[dvrogozh]

Decoder and encoder hw codec implementations which previously resided in the plugins were moved back to library. Plugins do still exist for the backward compatibility, but they are actually proxy to redirect everything to the library codec implementation.

Now, there are still 2 things to check/fix before samples implementation could be considered dropped:

1. While codecs are back in the library I am not 100% sure whether they start fully use internal mediasdk API and allocations. In a way this still can be an issue.
2. There is one remaining plugin - LookAhead one. Need to check which buffers it use.

[lizhong1008]

Quickly tried in ffmpeg mainline, removing loading hevc pluging. But hevc encoding still failed with an error: Unsupported format: pal8. Looks like the allocations haven't change, MFX_FOURCC_P8 is still be used. Would you please do some internal testing to make sure it works as expectation? And updating the encoding sample should be good.

[onabiull]

Please try to reproduce with actual codebase https://github.com/Intel-Media-SDK/MediaSDK/tree/master/samples and/or move issue to https://github.com/Intel-Media-SDK/MediaSDK. This repo would be archived.