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

[Mark Thompson]

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.

Thanks,

- Mark