cbaggers / cepl

Code Evaluate Play Loop
BSD 2-Clause "Simplified" License
860 stars 52 forks source link

Strange memory fault when pull-g on layers of array texture #336

Open cbaggers opened 5 years ago

cbaggers commented 5 years ago

This code is pretty reliable at triggering the issue after a few runs

(defun maybe-death ()
  (let* ((layer-count 7)
         (t0 (make-texture nil :dimensions '(4 4)
                           :element-type :float
                           :layer-count layer-count)))
    (loop
       :for i :below layer-count
       :collect (pull-g (texref t0 :layer i)))))
cbaggers commented 5 years ago

Oh this is super fucky! It turns out that glGetTexImage is trying to pull all the layers. Well that sucks..

I used glGetnTexImage so it would check the size and throw an error rather than just crashing.

So CEPL was making a c-array of the correct size for a single image but naturally that was far too small for this. I wonder what to do now I have a horrible feeling that only later version of gl supported getting a single layer

cbaggers commented 5 years ago

ooo, maybe this will help

Parameter Name                  Type       Initial Value    Valid Range
UNPACK_SWAP_BYTES               boolean    FALSE            TRUE/FALSE
UNPACK_LSB_FIRST                boolean    FALSE            TRUE/FALSE
UNPACK_ROW_LENGTH               integer    0                [0,∞)
UNPACK_SKIP_ROWS                integer    0                [0,∞)
UNPACK_SKIP_PIXELS              integer    0                [0,∞)
UNPACK_ALIGNMENT                integer    4                1,2,4,8
UNPACK_IMAGE_HEIGHT             integer    0                [0,∞)
UNPACK_SKIP_IMAGES              integer    0                [0,∞)
UNPACK_COMPRESSED_BLOCK_WIDTH   integer    0                [0,∞)
UNPACK_COMPRESSED_BLOCK_HEIGHT  integer    0                [0,∞)
UNPACK_COMPRESSED_BLOCK_DEPTH   integer    0                [0,∞)
UNPACK_COMPRESSED_BLOCK_SIZE    integer    0                [0,∞)
cbaggers commented 5 years ago

Yes! https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glPixelStore.xhtml

We can use skip images and row height to grab what we need.

cbaggers commented 5 years ago

ugh. maybe not. it looks like skip is about incrementing the pointer :|

metayan commented 5 years ago

No memory fault with the example here.

cbaggers commented 5 years ago

@metayan yeah I had some issues reproducing it at first, but glGetTexImage is writing into memory is doesnt own. If it's not crashing then it's more worrying :D

I expanded the example to this which uses %gl:getn-tex-image which throws a nice(r) error when called.

(defun often-death ()
  (let* ((layer-count 7)
         (t0 (make-texture nil :dimensions '(4 4)
                           :element-type :float
                           :layer-count layer-count)))
    (loop
       :for i :below layer-count
       :for src := (texref t0 :layer i)
       :collect
         (let* ((p-format (image-format->pixel-format
                           (cepl.textures::gpu-array-t-image-format src)))
                (c-array (make-c-array nil
                                       :dimensions (gpu-array-dimensions src)
                                       :element-type p-format
                                       :row-alignment 1)))
           (setf (gpu-buffer-bound (cepl-context) :pixel-pack-buffer) nil)
           (let ((row-alignment (cepl.c-arrays::c-array-row-alignment c-array)))
             (cepl.textures::with-gpu-array-t src
               (destructuring-bind (pix-format pix-type)
                   (cepl.pixel-formats::compile-pixel-format p-format)
                 (cepl.textures::%with-scratch-texture-bound texture
                   (setf (pack-alignment) row-alignment)
                   (%gl:getn-tex-image
                    texture-type
                    (coerce level-num '(signed-byte 32))
                    pix-format
                    pix-type
                    (cepl.c-arrays::c-array-byte-size c-array)
                    (c-array-pointer c-array))
                   ))))
           c-array))))
metayan commented 5 years ago

Noticed this difference:

(defglfun ("glGetTexImage" get-tex-image) :void
  (target enum)
  (level int)
  (format enum)
  (type enum)
  (pixels offset-or-pointer))
(defglextfun ("glGetnTexImage" getn-tex-image) :void
  (target enum)
  (level int)
  (format enum)
  (type enum)
  (bufSize sizei)
  (pixels (:pointer :void)))

No idea what it means, though. Just Zen-navigating.

cbaggers commented 5 years ago

https://stackoverflow.com/questions/32070930/copying-a-single-layer-of-a-2d-texture-array-from-gpu-to-cpu/32072660#32072660

What version of GL are you working with?

You are probably not going to like this, but... GL 4.5 introduces glGetTextureSubImage (...) to do precisely what you want. That is a pretty hefty version requirement for something so simple; it is also available in extension form, but that extension is relatively new as well.

There is no special hardware requirement for this functionality, but it requires a very recent driver.

I would not despair just yet, however.

You can copy the entire texture array into a PBO and then read a sub-rectangle of that PBO back using the buffer object API (e.g. glGetBufferSubData (...)). That requires extra memory on the GPU-side, but will allow you to transfer a single slice of this 2D array.

cbaggers commented 5 years ago

@metayan yeah with glGetnTexImage you give it the size of your buffer and it will throw a gl error if it isnt big enough. I then played with the dimensions to the c-array until it didnt crash and the sweet spot was 448 bytes.

That's (/ 448 16 4) gives us 7 which is the number of layers, and the point where I started swearing hehe

cbaggers commented 5 years ago

Time for me to sleep though, seeya :)

metayan commented 5 years ago

Ah, this macOS only reports OpenGL 4.1, so %gl:getn-tex-image isn't available, so can't try (often-death).

metayan commented 5 years ago

Same, bye.

cbaggers commented 5 years ago

Work progressing but still incorrect for cube textures