[FFmpeg-devel] [PATCH v08 2/3] fbtile tile/detile, hwcontext_drm detile NonLinear

[Lynne]

Jul 11, 2020, 08:52 by hanishkvc at gmail.com:

> ** fbtile cpu based framebuffer tile/detile helpers
>
> Add helper routines which can be used to tile/detile framebuffer
> layouts between linear and specified tile 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 fbtile_generic logic, which can be easily configured
> to support different kinds of tiling layouts, at the expense of some
> additional processing cycles, compared to developing custom (de)tiling
> logic. One requires to provide the tile walking parameters for the
> new tile layout to be supported. Once it is done, both tiling and
> detiling of the new tile layout can be handled automatically.
>
> This is non-public by default.
>

I thought I made myself clear. No compile time public header options.
That means no SCOPEIN, no HWCTXDRM_SYNCRELATED_FORMATMODIFIER,
no DEBUG_FBTILE_FORMATMODIFIER_MAPPING, no FBTILE_SCOPE_PUBLIC,
no FBTILER_OPTI_UNROLL (just always unroll), no FBTILER_GENERIC_OPTI,
none of those please, at all. No, this is not up for discussion.
I can't review this patch well at all with this.
You can depend on CONFIG_LIBDRM always being available, as we're never ever
going to be using this code outside of hwcontext_drm.c.
We align our switch cases with the switch indentation, so no extra indent. Its not a block.
Use av_log instead of av_log_once.
Also fix the changelog.
And please stop submitting the fbdetile filter patch, even for informative reasons.
Just dump it in a git repository somewhere.


> +// Common return values> +#define FBT_OK 0> +#define FBT_ERR 1

Really? Use 0 for ok and AVERROR... for errors.


> +/*> + * Copy one AVFrame into the other, tiling or detiling as required, if possible.> + * NOTE: Either the Source or the Destination AVFrame (i.e one of them) should be linear.> + * NOTE: If the tiling layout is not understood, it will do a simple copy.> + */

If the contents aren't going to change you can avoid copying by just using av_frame_ref.


Like I said, this is just a fraction of what I can review with all those ifdefs around the code.