Re: [igt-dev] [PATCH i-g-t v2 4/8] lib/igt_frame: Add function to dump frames in RGB and raw

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

On Wed, 4 May 2022 16:56:31 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

> Hi Pekka,
> 
> Thanks for reviewing those patches
> 
> On Fri, Apr 29, 2022 at 03:46:45PM +0300, Pekka Paalanen wrote:
> > On Mon, 28 Mar 2022 16:55:05 +0200
> > Maxime Ripard <maxime@cerno.tech> wrote:
> >   
> > > The igt_write_frame_to_png() already allows to dump the content of a
> > > cairo surface into a PNG image. However, it can be useful to have the
> > > raw content of the buffer as well, so let's create a function that will
> > > dump both a PNG image and its raw buffer.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>  
> > 
> > Hi,
> > 
> > when exactly is the raw dump useful? Do you need it for checking alpha?
> > Or for YUV formats? Or for multi-planar formats? Or for greater than 8
> > bpc or floating point formats?  
> 
> Anything, really :)
> 
> When I started debugging this, the only feedback I got from IGT was the
> hash it computed from the image, which isn't particularly useful for
> debugging.
> 
> > How do you make use of the raw dump?  
> 
> I'm using http://rawpixels.net/
> 
> > PNG can store alpha channel as well, and if that's not convenient
> > alongsize RGB, you could save another image where alpha has been
> > converted to gray scale.
> > 
> > PNG can also go up to 16 bpc I think?
> > 
> > PAM file format (netpbm type P7) might also be an option, looks like it
> > can do up to RGBA 16 bpc, and is trivial to generate.
> > http://netpbm.sourceforge.net/doc/pam.html
> > 
> > It's not much different from a raw dump, but contains enough metadata
> > for tools to understand the image.  
> 
> TIL :)
> 
> But I'd assume all those formats are still RGB, right? So if we ever go
> to check other formats, we'll get back into the same situation.

Yes, they are all RGB(A).

I think you have to get rid of Cairo first, before non-RGB can even be
a problem in file saving.

Cairo does not even do 16 bpc RGB(A):
https://www.cairographics.org/manual/cairo-Image-Surfaces.html#cairo-format-t
It only goes up to 10 bpc.

> That being said, I don't have a strong opinion for that patch (and the
> next one). I've found it to be useful when debugging, but I'd definitely
> understand if that's not something we want in IGT.

I'm not opposing this patch, I just wanted to know how it's useful.
Something to add to the commit message maybe?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]