Jul 5, 2020, 23:47 by s...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
