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

[Lynne]

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.