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

[C Hanish Menon]

Hi Mark,

On Mon, Jul 6, 2020 at 4:18 AM Mark Thompson <sw at jkqxz.net> wrote:

> 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 for the feedback. I thought your suggestion about
av_frame_copy_with_tiling was good. However as you and Lynne seem to have
discussed further and concluded not to expose a public api like
av_frame_copy_with_tiling, so the equivalent function which I had, which
has been further updated to support tiling also (in addition to detiling,
so that is a more fuller copy), I have named as fbtile_frame_copy (it is
limited to the 3 common Intel tile formats, the logic is extendable in a
easy way for new tile layouts) in my new patch.

In future if ffmpeg is implementing such an api, you may also want to add
an additional status argument, which will indicate as to whether the logic
did only a plain copy or a tile/detile based copy. As the user of the
function may want indication as to whether the tiling related operation was
done or not (may not be done, because the combination of
pixelformat+planes+tile layout involved may not be supported). At the same
time still retaining the simple 0 for success (a fall back plain copy can
still be a valid copy at one level) function return value.

Also you may require two additional public api

one) which maps from a hardware(and related) context specific tile layout
id to the tile layout id used by the ffmpeg avframe tile support logic.

two) which tells whether the combination of pixel format (+ planes) + tile
layout is supported or not.

Just for my understanding, if a library wants to log something, so that it
doesnt chain back corner case and or debug meta data to its users, what
should it do. Should it define a dummy class with a name field or some such
thing (NOTE: I havent really looked at av_log in detail, this is just a
guess based on some comment or so, which I vaguely remember reading related
to av_log or some other thing).


> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



-- 
Keep ;-)
HanishKVC