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

[C Hanish Menon]

Hi Lynne,

On Sat, Jul 11, 2020 at 5:20 PM Lynne <dev at lynne.ee> wrote:

> 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.
>

Reason HWCTXDRM_SYNCRELATED_FORMATMODIFIER is provided is because currently
as the AVFrame doesn't have any generic tiling info related member, so the
related HW AVFrame is currently used to notify any other users in future
within hwcontext_drm that it has been detiled. But then again has there is
no single right or wrong way here at one level, so I put it under a ifdef
block and what I felt as a potentially sane default for now was set for the
same.

The DEBUG defs (DEBUG_FBTILE and DEBUG_...MAPPING) are provided so that in
future, if someone wants to debug the flow for some reason like say a new
tile layout being added or some other reason, then they don't have to hunt
as to where to put debug prints to help understand the flow, instead they
could use the already available compile time option to enable the debug
logs and then get one level of understanding. And again currently a sane
default of keeping it disabled (undef'd) is used.

The reason UNROLL is put within ifdef is because if for some reason a
tiling layout which involves less than 4x4 subtile block is required to be
supported in future, then the unrolling will need to be avoided in this
function or a new function without unrolling will have to be created. Again
to help a future developer who may face such a situation, the code is
properly identified and put into suitable ifdef blocks and suitably
commented in the code. Again a sane default which is fine for the current
situation is setup.

Again the idea of FBTILE_SCOPE_PUBLIC, is so that a logically independent
code, can be embedded into a some other code in a structured way, while at
same time providing the flexibility to move it into a independent public
api if required in future, if another use case for the logic comes up.

I do agree that these compile time options won't be changed in a normal
compile, but then again it is not for a normal compile situation, but for
future flow changes if required in future for a developer (or a user who
wants to experiment) who may have to touch the code.

I am not sure your way of thinking of trying to structure everything to an
immediate specific need, is the right way of approaching things in general,
when the underlying logic is in reality independent and even when things
are potentially (I do accept different people may implement the things in
slightly different ways) structured in a sane way to provide future
flexibility.

At the same time I appreciate and am thankful to the volunteering you and
other developers are doing here and will change the code to be more
specific if you still feel that is what you, as an active ffmpeg developer,
feel.

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.
>

As what I have mentioned above already, you seem to want to structure the
code to be very specific to the current immediate use case, while I
normally prefer structuring things to be flexible and adaptable for the
unknown future, especially when the logic in question is in reality
independent at one level.

Also in this case I feel it is a sensible ifdef'd block for drm specific
code (to map from format_modifier to the logic's id for the tiling layout)
in an otherwise generic logic and we should retain it.


> We align our switch cases with the switch indentation, so no extra indent.
> Its not a block.
>

Understood the ffmpeg convention, will update.


> Use av_log instead of av_log_once.
>

In a video flow, has the same unwanted setup can trigger more than once in
a single run, like say an unknown tile layout, in such a situation I didnt
want to flood the 'by-default' user visible log with message about the same
more than once, so I used av_log_once and inturn changed the log level
between the 1st and subsequent log entries. Isn't it what one is expected
to do in such a situation, or am I missing something.


> Also fix the changelog.
>

Sorry I didn't understand this fully. Can I assume that you don't want to
mention about the fbtile helpers as an entry in the changelog, because its
not a public api? From my perspective the reason why I added it to the
changelog is so that if someone is looking at availability of some
framebuffer tiling/detiling support in future, the changelog notes the same
in a simple way. However if you feel it's not needed I will remove it. Or
did you mean something else?


> And please stop submitting the fbdetile filter patch, even for informative
> reasons.
> Just dump it in a git repository somewhere.
>
>
As was being discussed in another thread in the mailing list, currently
there is no central registry of available video filters out there, so dont
you think mailing list is the next best location to have the source patch
for the filter, for someone who may (or may not) look for things in future?
Just asking and thinking out loud.


>
> > +// Common return values> +#define FBT_OK 0> +#define FBT_ERR 1
>
> Really? Use 0 for ok and AVERROR... for errors.
>
> Will change ;-)


>
> > +/*> + * 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.
>
>
Thanks for suggesting the same (as previously mentioned, I haven't worked
on ffmpeg before so am not aware of all the ffmpeg helper routines, and did
this code more to solve a specific use case I had and parallely wanted to
have some fun implementing the logic, as well as contribute something
possibly useful back to ffmpeg as a thank you to all involved).

However at the same time, when used for example like in hwcontext_drm, the
underlying source is a specific hardware's mmap'd memory, so should one
pass the source reference along to the destination (i am assuming that is
what av_frame_ref is, have to check still), or shouldn't one move the data
out of the hardware specific memory, as early as possible.


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

Thanks once again for providing the feedback, and sorry for all the
ifdef's I have put in, I have specified the reason for some of those ifdefs
above, based on your response, I will remove most of them, if that is what
you feel is right.


> _______________________________________________
> 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