[Mesa-stable] [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Oded Gabbay]

On Sat, Aug 8, 2015 at 8:38 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>> This patch fixes a bug that is manifested in the read path of mesa when
>>> running on big-endian machines. The effects can be seen when running
>>> piglit sanity test and/or taking a screen capture.
>>
>> piglit sanity isn't all that convincing.  It's quite possible that
>> there are two byte-swapping bugs that just happen to cancel out.  If
>> it fixes huge numbers of piglit tests, that's more convincing.
>>
>
> Well, without it about 65% of piglit tests fails. With this fix, about
> 30% fails. I would say that accounts for something...
>
>>> The bug is caused when _mesa_format_convert receives src_format as
>>> mesa_format, which it thens changes to mesa_array_format. During this
>>> change, it checks for endianness and swaps the bytes accordingly.
>>> However, because the bytes are _already_ swapped in the memory itself
>>> (being written there by llvmpipe/softpipe/sw rast), and src_format
>>> value matches the _actual_ contents of the memory, the result of the
>>> read is wrong.
>>>
>>> Therefore, because other layers/functions, such as llvm or
>>> read_rgba_pixels() takes care whether we are dealing with big-endian or
>>> little-endian, _mesa_format_convert should be endian agnostic to avoid
>>> duplicate swapping of bytes.
>>
>> As far as I know, the core mesa format conversion code has handled
>> byte-swapping for a very long time.  it is *not* simply punted to the
>> driver.  I'm 100% ready to believe that there is a problem but it's
>> probably far more subtle than just removing support for byte-swapping
>> in _mesa_format_to_array_format.
>
> First of all, this bug has been in a very long time as well (I tracked
> it to between 10.4 to 10.5). It has shown up since mesa started to use
> _mesa_format_convert
> From my debugging, I see that llvmpipe (and other) write the data to
> memory in BE format. e.g if the format is RGBA, then the data is
> actually written in AGBR format.
> Now, in _mesa_format_convert you get the src_format as AGBR (already
> swapped). However, because it arrives as format and not array_format,
> _mesa_format_convert needs to build the array_format in
> _mesa_format_to_array_format()
>
> In _mesa_format_to_array_format, after the array_format is created,
> you swap it if it is BE, so you now have RGBA in array_format
> representing the source. And that is the bug!
>
> of course I checked that _mesa_format_to_array_format() is only called
> from _mesa_format_convert so I can safely modify it according to
> _mesa_format_convert usage.
>
> So I don't think it is "far more subtle" than removing this
> byte-swapping. If you still think so, please elaborate.
>
> Thanks,
>          Oded
>
Two other things:

1. You can see the effects of this fix by taking a screen capture.
Without this fix, the screen capture image is all wrong - all colors
are inverted. With this fix, the screen capture is fine.

2. If you are interested, the exact timing of this bug appearance was
in this commit: 5038d839b8e4d5926ee2aeaa4a66367ba99a31c6 mesa: use
_mesa_format_convert to implement glReadPixels.

Interestingly, this commit was r-b by you, Jason...

    Oded



>>
>>> btw, it is also mentioned in the comment of the function that it doesn't
>>> handle byte-swapping, so the original code contradicts the documentation.
>>>
>>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>>> CC: "10.5 10.6" <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>>> ---
>>>  src/mesa/main/formats.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
>>> index baeb1bf..7a41cb0 100644
>>> --- a/src/mesa/main/formats.c
>>> +++ b/src/mesa/main/formats.c
>>> @@ -372,10 +372,8 @@ uint32_t
>>>  _mesa_format_to_array_format(mesa_format format)
>>>  {
>>>     const struct gl_format_info *info = _mesa_get_format_info(format);
>>> -   if (_mesa_little_endian())
>>> -      return info->ArrayFormat;
>>> -   else
>>> -      return _mesa_array_format_flip_channels(info->ArrayFormat);
>>> +
>>> +   return info->ArrayFormat;
>>>  }
>>>
>>>  static struct hash_table *format_array_format_table;
>>> --
>>> 2.4.3
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev