<div dir="ltr">Hi Merek,<div><pre style="margin-top:0px;margin-bottom:0px"> </pre></div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Jan 22, 2014 at 2:49 PM, Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Mark, Could you please mention or document somewhere in the code (e.g. in main/formats.h) which _REV formats are incorrect according to you? Sorry if you did so already, I haven't read your other patches yet. </blockquote><div> </div><div>There was not a universally correct solution for some of the _REV MESA_FORMATs thus I defaulted to the OGL definition of _REV when eliminating the _REV decoration. For the examples below, MESA_FORMAT's use of _REV sometimes differs from what is described in the OGL pixel transfer doc, which says the following about GL_UNSIGNED_SHORT_5_6_5, for example:</div> <div>"This can have a _REV at the end of it. This would mean to reverse the order of the components in the data. In "REV" mode, the first component, the one that matches the first 5, would go into the last component specified by the format." </div> <div> </div><div>Using _REV with MESA_FORMATs to have a big and little endian representation of a format agrees with the OGL definition most of the time but not for formats where each color components isn't equally divided on a byte boundary: </div> <div> </div><div><pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_RGB565, /* RRRR RGGG GGGB BBBB */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_RGB565_REV, /* GGGB BBBB RRRR RGGG */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_ARGB4444, /* AAAA RRRR GGGG BBBB */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_ARGB4444_REV, /* GGGG BBBB AAAA RRRR */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_RGBA5551, /* RRRR RGGG GGBB BBBA */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_ARGB1555, /* ARRR RRGG GGGB BBBB */</pre> <pre style="margin-top:0px;margin-bottom:0px"> MESA_FORMAT_ARGB1555_REV, /* GGGB BBBB ARRR RRGG */</pre> <pre style="margin-top:0px;margin-bottom:0px"> </pre><pre style="margin-top:0px;margin-bottom:0px">Searching all of Mesa for the above _REV formats reveals few hits within the drivers that are actually employing the endianess interpretation of _REV, vs the OGL interpretation. For example radeon_screen.c:</pre> <pre style="margin-top:0px;margin-bottom:0px"><pre style="margin-top:0px;margin-bottom:0px"> rgbFormat = _mesa_little_endian() ? MESA_FORMAT_RGB565 : MESA_FORMAT_RGB565_REV;</pre> <pre style="margin-top:0px;margin-bottom:0px"> </pre></pre><pre style="margin-top:0px;margin-bottom:0px">vs radeon_pixel_read.c</pre><pre style="margin-top:0px;margin-bottom:0px"> <pre style="margin-top:0px;margin-bottom:0px"> case GL_UNSIGNED_SHORT_5_6_5:</pre> <pre style="margin-top:0px;margin-bottom:0px"> return MESA_FORMAT_RGB565;</pre> <pre style="margin-top:0px;margin-bottom:0px"> case GL_UNSIGNED_SHORT_5_6_5_REV:</pre> <pre style="margin-top:0px;margin-bottom:0px"> return MESA_FORMAT_RGB565_REV;</pre> <pre style="margin-top:0px;margin-bottom:0px"> </pre><pre style="margin-top:0px;margin-bottom:0px">In many of the cases that rely on the endianess interpretation, the _REV decoration is ignored anyway.</pre> <pre style="margin-top:0px;margin-bottom:0px"> </pre><pre style="margin-top:0px;margin-bottom:0px">So, based on your request, I should add a comment above the applicable format_unpack and format pack functions that says "this function does not match the current Mesa definition of <insert format here>". Is that acceptable? With the latest patch set I just buried my head in the sand on this one.</pre> <pre style="margin-top:0px;margin-bottom:0px"> </pre><pre style="margin-top:0px;margin-bottom:0px"> </pre></pre></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> Also, I have a proposal for SRGB formats. MESA_FORMAT_SRGB_UNORM8 and MESA_FORMAT_SA8B8G8R8_UNORM look weird, because they are not really UNORM and there is also no stencil. :) How about this: MESA_FORMAT_RGB_SRGB8 (denoting an array format of the SRGB type and 8 bits per channel) and MESA_FORMAT_A8B8G8R8_SRGB (denoting a packed format of the SRGB type). </blockquote><div> </div><div>I agree it could be better. My reasoning for avoiding what you suggested is that it creates an ambiguity between color information and storage type. For instance, sRBG color space values could feasibly be stored as floats, half floats, snorms, or unorms, could they not? Thus the S is a color component modifier, not storage type. Would it be too awkward to use M for stencil mask instead of S? Is there a clearer method that doesn't hide storage type information?</div> <div> </div><div> Mark</div></div></div></div>