[FFmpeg-devel] [PATCH v06 2/5] fbtile helperRoutines cpu based framebuffer detiling

Mark Thompson sw at jkqxz.net
Tue Jul 7 01:13:42 EEST 2020


On 06/07/2020 00:30, Lynne wrote:
> Jul 5, 2020, 23:47 by sw at jkqxz.net:
> 
>> On 04/07/2020 14:17, hanishkvc wrote:
>>
>>> Add helper routines which can be used to detile tiled framebuffer
>>> layouts into a linear layout, using the cpu.
>>>
>>> Currently it supports Legacy Intel Tile-X, Legacy Intel Tile-Y and
>>> Newer Intel Tile-Yf tiled layouts.
>>>
>>> Currently supported pixel format is 32bit RGB.
>>>
>>> It also contains detile_generic logic, which can be easily configured
>>> to support different kinds of tiling layouts, at the expense of some
>>> processing speed, compared to developing a targeted detiling logic.
>>> ---
>>>   libavutil/Makefile |   2 +
>>>   libavutil/fbtile.c | 441 +++++++++++++++++++++++++++++++++++++++++++++
>>>   libavutil/fbtile.h | 228 +++++++++++++++++++++++
>>>   3 files changed, 671 insertions(+)
>>>   create mode 100644 libavutil/fbtile.c
>>>   create mode 100644 libavutil/fbtile.h
>>>
>>
>> I do think this is a reasonable thing to include in FFmpeg, but please think about what you actually want as a public API here.
>>
>> Ideally you want to minimise the public API surface while providing something clean and of general use.
>>
>> So:
>> - It should probably operate on whole frames - copying pieces of frames or single planes doesn't have any obvious use.
>> - The pixel format, width, height and pitches will all need to be specified, so AVFrames which already include that information are probably the right structure to use.
>> - You want to specify a tiling mode for the source frame somehow.
>> - For the untile case the destination is linear, but a plausible use-case is the opposite so we could include tiling mode for the destination as well.
>> - The tiling modes will need to be some sort of enum, since they are all ad-hoc setups for particular hardware vendors.
>> - We can't reuse existing values like those from libdrm because we'd like it to work everywhere it can and there is no intrinsic dependence on libdrm, so it needs to be a new enum.
>> - Names for the tiling modes should be valid everywhere, so if they are platform-dependent (like Intel X/Y tiling) then the platform will need to be included in the name.
>> - Linear should be in the tiling mode enum, so that you don't need special cases elsewhere.
>> - All the dispatch between different implementations can happen internally, so it doesn't need to be exposed.
>> - Not everything will actually be implemented, so it will need to be able to return an error indicating that a particular case is not available.
>>
>> Given that, I make the desired public API to be exactly one enum and one function.  It would look something like:
>>
>> enum AVTilingMode {
>>   AV_TILING_MODE_LINEAR = 0,
>>   AV_TILING_MODE_INTEL_GEN7_X,
>>   AV_TILING_MODE_INTEL_GEN7_Y,
>>   AV_TILING_MODE_INTEL_GEN9_X,
>>   AV_TILING_MODE_INTEL_GEN9_Y,
>>   AV_TILING_MODE_INTEL_YF,
>> };
>>
>> int av_frame_copy_with_tiling(const AVFrame *dst, enum AVTilingMode dst_tiling,
>>   const AVFrame *src, enum AVTilingMode src_tiling);
>>
>>
>> Some other thoughts:
>> - Functions should all be static unless you are intentionally exposing them as public API.
>> - Libraries must not include mutable globals.
>> - Note that av_log(NULL, ...) should never be called from a library unless you really are in a global context.  I think you probably just don't want to include any user-facing logging at all.
>> - Look at the coding style guide, especially around naming and operator spacing.
>>
> 
> Yes, in general those are good guidelines for public APIs.
> But please, _don't_ in this case. Really. I've already said its not a good idea and I'll reject
> future patches that do simply because its a problem exclusive to hwcontext_drm and directly
> solvable there.

Yeah, that's fair - there isn't really any good reason to make it public unless there is another use-case entirely independent from the DRM one.

- Mark


More information about the ffmpeg-devel mailing list