Store mipmap levels in deferred texture image

BUILD.gn

Index: BUILD.gn
diff --git a/BUILD.gn b/BUILD.gn
index ff3c492ffde7e72d7f8d4df9fa836e172e42ec04..0ffa543e44081ef61e1fce6fc59f4c31668eb7bd 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -543,6 +543,7 @@ if (!is_component_build) {  # Our test tools use many non-SK_API APIs...
       "dm/DMJsonWriter.cpp",
       "dm/DMSrcSink.cpp",
     ]
+    cflags = [ "-fno-strict-aliasing" ]

[cblume, 2016/08/12 22:25:35]

I believe the problem as pointer aliasing.
This solves it, but in a way I do not like. I'm only using it to point out that
this seems to be the issue.

This makes GCC's optimization less aggressive.

[mtklein, 2016/08/14 14:06:24]

Right, we turn on -fstrict-aliasing in our test bots even more often than GCC
does naturally, to be compatible with clients who may use it.  It's the same
reason we turn off RTTI and exceptions.  To my knowledge, though, the clients we
pay most attention to disable strict aliasing because it's too hard or subtle a
constraint for developers to manage.  I tend to agree, except that's pretty
clear you want to be able to assume nothing aliases 'this'.  If the compilers
were better about warning about when a strict-aliasing related optimization
fired, it'd also be less of an issue... mostly the problem is it's a silent
killer.

Are you sure this here fixes your problem?  This flag will apply only to those 3
files listed above, right?  And only if you're using GN... most people are using
GYP.

SkTCast looks well-intentioned but, I think, incorrect.  That union is a fine
implementation-defined way of aliasing its elements (non-standard, but supported
by the compilers we use).  However, it doesn't guarantee anything about the
aliasing of the pointed-to elements.  It's OK to use a union to pun a float to
an int (e.g. SkFloatIntUnion), but it doesn't help you to pun a float* to and
int*.

memcpy() is always a safe choice, but again it's got to be done on the data
itself, not as a roundabout reinterpret_cast on the pointer.  It's not helpful
if you want to pun the data in-place.

The only other way I know to fix this is to introduce an aliased variant of the
type you're working with, using __attribute__((may_alias)).  It bestows on that
type the same special property that char has, which lets memcpy() actually be
implemented.  And this is how GCC manages its own aliased builtin types, like
__m128i.  You can take a look at SkChecksum_opts.h:91 to see may_alias in
action.  To my knowledge, only GCC does dirty things with aliasing... Clang will
accept the attribute but already wasn't optimizing in any unexpected way, and
MSVC doesn't seem to try to exploit strict aliasing in this way either.

When you say,
  "BTW, we have it in other spots as well. When testing locally I was able to
get other spots to trigger."
can you list those other spots, or show us how you found them?  I'd like to get
bots up to guard against this.

[cblume, 2016/08/15 23:39:47]

Oh huh. Maybe the bot runs passed just by chance then. If this only effects
those 3 files then this shouldn't fix anything.
I agree. I just took a look at SkTCast and I'm uncomfortable with it. When you
write one type of a union, you can only legally read from that type. Anything
else is undefined behavior. But SkTCast uses the C behavior (not C++ behavior)
where you can use a union in this way.

As far as I know, all the major compilers don't take advantage of this undefined
behavior in any way and they behave like C does. But I'm still not a huge fan. I
guess it would at least put the undefined behavior in one place so if it becomes
a problem you can update all call sites.

But all that aside, you are right that it would still be aliasing when used with
pointers.
I agree. I still feel confident the problem is aliasing. It is only showing up
on GCC builds. Even if my -fno-strict-aliasing didn't apply.
Oh, I thought this was an access violation but it might actually be an unaligned
pointer plus an instruction that requires aligned data.

In the stack below you'll see dst was 0xb399ab14 (not 16-byte aligned) and it
looks like dst1 = dst.
https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkTextureCompr...

Get a fresh checkout of Skia (no changes)
These instructions are for Debug but it happens in Release as well.

$ adb devices
List of devices attached
(my nexus 6 serial here)      device
(Just to sanity check that I'm not targeting the wrong device)

$ ./platform_tools/android/bin/android_ninja -d nexus_6 -t Debug --gcc

$ ./platform_tools/android/bin/android_gdb_native --debug dm --resourcePath
/data/local/tmp/skia/resources

Thread 2 "skia_launcher" received signal SIGBUS, Bus error.
[Switching to Thread 6837.6931]
0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
9161      __builtin_neon_vst1v2di ((__builtin_neon_di *) __a, (int64x2_t) __b);
(gdb) info stack
#0  0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
#1  neon::compress_r11eac_blocks (dst=0xb399ab14, src=0xb394b800 "",
rowBytes=48)
    at ../../../../src/opts/SkTextureCompressor_opts.h:208
#2  0xb5cdca0e in neon::compress_a8_r11eac (dst=0xb399ab14 "\377",
src=0xb394b800 "", 
    width=48, height=48, rowBytes=48)
    at ../../../../src/opts/SkTextureCompressor_opts.h:231
#3  0xb5eaf2b2 in SkTextureCompressor::CompressBufferToFormat (dst=0xb399ab14
"\377", 
    src=0xb394b800 "", srcColorType=kAlpha_8_SkColorType, width=48, height=48, 
    rowBytes=48, format=SkTextureCompressor::kR11_EAC_Format)
    at ../../../../src/utils/SkTextureCompressor.cpp:125
#4  0xb5eaf3ec in SkTextureCompressor::CompressBitmapToFormat (pixmap=..., 
    format=SkTextureCompressor::kR11_EAC_Format)
    at ../../../../src/utils/SkTextureCompressor.cpp:158
#5  0xb4c60c58 in test_CompressCheckerboard (reporter=0xb6d78d04)
    at ../../../../tests/TextureCompressionTest.cpp:157
#6  0xb4946176 in run_test (test=...) at ../../../../dm/DM.cpp:1248
#7  0xb49462e6 in <lambda()>::operator()(void) const (__closure=0xb67ff070)
    at ../../../../dm/DM.cpp:1347
#8  0xb494751a in std::_Function_handler<void(), dm_main()::<lambda()>
>::_M_invoke(const std::_Any_data &) (__functor=...)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2039
#9  0xb5d697b0 in std::function<void ()>::operator()() const (this=0xb6d78d54)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2439
#10 0xb5d68e30 in (anonymous namespace)::ThreadPool::Loop (arg=0xb646f070)
    at ../../../../src/core/SkTaskGroup.cpp:167
#11 0xb5eb8c5c in thread_start (arg=0xb647e040)
    at ../../../../src/utils/SkThreadUtils_pthread.cpp:66
#12 0xb6f69bb0 in __pthread_start(void*) ()
   from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
#13 0xb6f67af4 in __start_thread ()
   from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
#14 0x00000000 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

[mtklein, 2016/08/16 02:22:22]

Yes, I agree the proximal problem here is that vst1q_u64() requires 8-byte
alignment, and 0xb399ab14 only has 4.  I bet you'll find 50%+ of runs pass
without changing anything.  :)

A vreinterpretq_uxx_u64() and using vst1q_u32() or vst1q_u8() will probably fix
this.

I just had to disable some unit tests of this texture compression code today. 
Is it used?

[mtklein, 2016/08/16 02:22:22]

So glad to have you on board, but I don't think this is quite the right way to
think about this.

Using a a field of a union that's not the most recently assigned is undefined
behavior in C and C++.  GCC and MSVC and Clang all break with the standard and
define this behavior as what you think it'd do.  The C++11 standard has chilled
out a little about what can go in unions, but is still totally uptight about
mixed field access, man.

MSVC and Clang seem to take their union trick further an extend this to
reinterpret_casts and C-style casts.  As far as I've seen, they do what you'd
think they do there.  GCC seems willing to apply strict aliasing rules here even
when it can clearly see you're asking to violate them.

All these compilers will happily let you pun float to int with a union.  GCC
will let you pun float* to int*, but now strict aliasing (of pointers) gets
involved and GCC invokes the "that's undefined, therefore didn't happen" rule if
you dereference that int*.  False implies true, pigs fly, the compiler does
whatever it wants.

[cblume, 2016/08/16 03:47:19]

Mmmm I think C and C++ differ in their union behavior.
http://en.cppreference.com/w/c/language/union (notice, this is the C area of
cppreference)

"If the member used to access the contents of a union is not the same as the
member last used to store a value, the object representation of the value that
was stored is reinterpreted as an object representation of the new type (this is
known as type punning)."


But I didn't know that MSVC/GCC/Clang all formally stated they would define the
behavior. I thought it just acted as expected but we weren't promised that. It
is nice to know they clarified this. :)

[cblume, 2016/08/16 03:47:19]

I have no idea if it is used. I was running into this (and also an illegal
instruction) via DM when trying to reproduce my error.


Speaking of, my error seemed to reproduce reliably every time on the bots
(making the revert clear). But I cannot reproduce it locally at all.

     include_dirs = [ "tests" ]
     deps = [
       ":experimental_svg_model",