On 06/07/2020 00:30, Lynne wrote:
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.
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
_______________________________________________
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".
