google / wuffs

Wrangling Untrusted File Formats Safely
Other
4.16k stars 131 forks source link

example/{imageviewer,sdl-imageviewer} don't work/work wrong with DefaultDepth 30 #51

Closed pjanx closed 3 years ago

pjanx commented 3 years ago

System: Arch Linux, X11 with DefaultDepth 30 Tested on various PNG images.

sdl-imageviewer always crashes, because SDL_GetWindowSurface() returns NULL.

imageviewer garbles the images it manages to display in the following way: image

On slightly larger images, it crashes on xcb_wait_for_event() returning NULL.

pjanx commented 3 years ago
diff --git a/example/sdl-imageviewer/sdl-imageviewer.cc b/example/sdl-imageviewer/sdl-imageviewer.cc
index 6d2b0fec..76819716 100644
--- a/example/sdl-imageviewer/sdl-imageviewer.cc
+++ b/example/sdl-imageviewer/sdl-imageviewer.cc
@@ -229,6 +229,10 @@ bool g_load_via_sdl_image = false;
 bool  //
 draw(SDL_Window* window) {
   SDL_Surface* ws = SDL_GetWindowSurface(window);
+  if (!ws) {
+    fprintf(stderr, "draw: SDL_GetWindowSurface(): %s\n", SDL_GetError());
+    return false;
+  }
   SDL_FillRect(ws, NULL, SDL_MapRGB(ws->format, 0x00, 0x00, 0x00));
   if (g_image) {
     SDL_BlitSurface(g_image, NULL, ws, NULL);

gives me the somewhat expected:

draw: SDL_GetWindowSurface(): Unknown window pixel format
pjanx commented 3 years ago

SDL can be made to work in the following manner, which even seems to be an acceptable patch as-is:

diff --git a/example/sdl-imageviewer/sdl-imageviewer.cc b/example/sdl-imageviewer/sdl-imageviewer.cc
index 6d2b0fec..1464c94c 100644
--- a/example/sdl-imageviewer/sdl-imageviewer.cc
+++ b/example/sdl-imageviewer/sdl-imageviewer.cc
@@ -229,6 +229,15 @@ bool g_load_via_sdl_image = false;
 bool  //
 draw(SDL_Window* window) {
   SDL_Surface* ws = SDL_GetWindowSurface(window);
+  if (!ws) {
+    SDL_Renderer *r = SDL_CreateRenderer(window, -1, 0);
+    SDL_Texture *texture = SDL_CreateTextureFromSurface(r, g_image);
+    SDL_RenderCopy(r, texture, nullptr, nullptr);
+    SDL_RenderPresent(r);
+    SDL_DestroyTexture(texture);
+    SDL_DestroyRenderer(r);
+    return true;
+  }
   SDL_FillRect(ws, NULL, SDL_MapRGB(ws->format, 0x00, 0x00, 0x00));
   if (g_image) {
     SDL_BlitSurface(g_image, NULL, ws, NULL);
pjanx commented 3 years ago

Feel free to close my issue once SDL works (just handle the failing SDL_GetWindowSurface), XCB rejected my attempts at debugging, and it's a horrible mess all around.

nigeltao commented 3 years ago

Thanks for the bug report. Can you copy/paste the output of xdpyinfo?

Or, if that's too large, perhaps xdpyinfo | head -n 100?

System: Arch Linux, X11 with DefaultDepth 30

Just double-checking... is it really 30, not 32?

pjanx commented 3 years ago

It's just 10-bit colour. xdpyinfo.log ~doesn't contain alpha, so it isn't all that useful~. This information is in pixelformats, not visuals.

Here is an XGB image viewer for a known-to-work approach, with XRender scaling, although it's set up to require the depth--you could have two serialising functions, one for ARGB32, another for RGB30.

pjanx commented 3 years ago

Coming back with randomly acquired knowledge. The display issue with the XCB demo is trivial: DecodeImageCallbacks::SelectPixfmt virtual-defaults to WUFFS_BASE__PIXEL_FORMAT__BGRA_PREMUL, and the xcb_image_t is hardcoded to use s->root_depth and completely disregards the Visual. The XRender extension could be used to resolve the difference within X.org, similarly to the way it's used in that Go code--this is the simplest fix, together with the SDL workaround.

The crash is due to XCB_CONN_CLOSED_REQ_LEN_EXCEED:

maximum request size:  16777212 bytes

which leaves shared memory or incremental uploads. I thought BIG-REQUESTS would help, but no.