2

Goal:

  • Send an Array of a given struct from C to Golang using cgo.

Working Code (no arrays)

Disclaimer: This is my first functional C code, things may be wrong.

GetPixel.c

#include "GetPixel.h"
#include <stdio.h>
#include <stdlib.h>
#include <X11/Xlib.h>
#include <X11/Xutil.h>

Display *display;
XImage *im;

void open_display()
{
    // xlib: must be called before any other X* methods, enables multithreading for this client
    XInitThreads();

    // save display in a global variable so we don't allocate it per `get_pixel`
    // TODO: figure out classes later (or are these C++ only? Can I use them in Golang?)
    display = XOpenDisplay((char *) NULL);
}

void close_display()
{
    // xlib: use XCloseDisplay instead of XFree or free for Display objects
    XCloseDisplay(display);
}

void create_image(int x, int y, int w, int h)
{
    // save image in a global variable so we don't allocate it per `get_pixel`
    im = XGetImage(display, XRootWindow(display, XDefaultScreen(display)), x, y, w, h, AllPlanes, XYPixmap);
}

void destroy_image()
{
    // xlib: use XDestroyImage instead of XFree or free for XImage objects
    XDestroyImage(im);
}

void get_pixel(struct Colour3 *colour, int x, int y)
{
    // TODO: could I return `c` without converting it to my struct?
    XColor c;
    c.pixel = XGetPixel(im, x, y);
    XQueryColor(display, XDefaultColormap(display, XDefaultScreen(display)), &c);

    // xlib: stored as values 0-65536
    colour->r = c.red / 256;
    colour->g = c.green / 256;
    colour->b = c.blue / 256;
}

GetPixel.h

# Trial and Error:
# - Golang needs me to define crap in an H file
# - C needs me to define my struct in an H file
# - C needs me to use `typedef` and name my struct twice (???)

#ifndef __GETPIXEL_
#define __GETPIXEL_

typedef struct Colour3 {
  int r, g, b ;
} Colour3 ;              # redundant?

void open_display();
void close_display();

void create_image(int x, int y, int w, int h);
void destroy_image();

void get_pixel(struct Colour3 *colour, int x, int y);

#endif

GetPixel.go

package x11util

func Screenshot(sx, sy, w, h int, filename string) {
    img := image.NewRGBA(image.Rectangle{
        image.Point{sx, sy}, image.Point{w, h},
    })

    defer trace(sx, sy, w, h, filename)(img)

    C.open_display()
    C.create_image(C.int(sx), C.int(sy), C.int(w), C.int(h))
    defer func() {
        # does this work?
        C.destroy_image()
        C.close_display()
    }()

    # Trial and Error
    # - C needs me to pass a pointer to a struct
    p := C.Colour3{}
    for x := sx; x < w; x++ {
        for y := sy; y < h; y++ {
            C.get_pixel(&p, C.int(x), C.int(y))
            img.Set(x, y, color.RGBA{uint8(p.r), uint8(p.g), uint8(p.b), 255})
        }
    }

    f, err := os.Create(filename)
    if err != nil {
        log.Error("unable to save screenshot", "filename", filename, "error", err)
    }
    defer f.Close()
    png.Encode(f, img)
}

Sure, it works -- but takes between 30 and 55 seconds for a 1080p screen.


Attempt 2

All the existing code, but instead of get_pixel, let's try to get patches of (3x3) 9 pixels at a time, as an optimization.

GetPixel.c

# ... existing code ...

struct Colour3* get_pixel_3x3(int sx, int sy)
{
    XColor c;

    # the internet collectively defines this as "a way to define C arrays where the data remains after the function returns"
    struct Colour3* pixels = (struct Colour3 *)malloc(9 * sizeof(Colour3));

    for(int x=sx; x<sx+3; ++x)
    {
        for(int y=sy; y<sy+3; ++y)
        {
            # ... existing code from Attempt 1 ...

            // Is this even the correct way to into C arrays?
            pixels++;
        }
    }

    return pixels;
}

GetPixel.h

# ... existing code ...
struct Colour3* get_pixel_3x3(int sx, int sy);

GetPixel.go

package x11util

func ScreenshotB(sx, sy, w, h int, filename string) {
    # ... existing code ...

    var i int
    for x := sx; x < w; x += 3 {
        for y := sy; y < h; y += 3 {
            // returns an array of exactly 9 pixels
            p := C.get_pixel_3x3(C.int(x), C.int(y))

            // unsafely convert to an array we can use -- at least, this was supposed to work, but never did
            // pb := (*[9]C.Colour3)(unsafe.Pointer(&p))

            // convert to a slice we can use... with reflect -- this code is magic, have no idea how it works.
            var pa []C.Colour3
            sliceHeader := (*reflect.SliceHeader)((unsafe.Pointer(&pa)))
            sliceHeader.Cap = 9
            sliceHeader.Len = 9
            sliceHeader.Data = uintptr(unsafe.Pointer(&p))

            // assign pixels from base (adding an offset 0-2) to print 3x3 blocks
            for xo := 0; xo < 3; xo++ {
                for yo := 0; yo < 3; yo++ {
                    img.Set(x+xo, y+yo, color.RGBA{uint8(pa[i].r), uint8(pa[i].g), uint8(pa[i].b), 255})
                    i++
                }
            }

            i = 0
        }
    }

    # ... existing code ...
}

This takes 20% less time to execute, the optimization is definitely there-- HOWEVER: the resulting image is a mess of either pink or yellow pixels, instead of the expected result. Which leads me to believe that I am reading random memory instead of my intention.

Since I know that reading a single pixel works and that the C loop works, I can only think that I totally misunderstand arrays in C, or how to pass them to Golang, or how to read/iterate them in Golang.


At this point, I have no idea what else to try, four pages of Stack Overflow and 20-ish pages of Googling has given me lots of different answers for the other direction (Go->C) -- but not a whole lot here. I could not find an example of C.GoBytes that worked either.


Attempt 3

Letting Golang handle allocation has simplified accessing the array. This code now works for 3x3, but fails when attempting to get "the whole screen at once" (see Attempt 4)

GetPixel.c

# ... existing code from Attempt 1 ...

# out-param instead of a return variable, let Golang allocate
void get_pixel_3x3(struct Colour3 *pixels, int sx, int sy)
{
    XColor c;
    for(int x=sx; x<sx+3; ++x)
    {
        for(int y=sy; y<sy+3; ++y)
        {
            # ... existing code from Attempt 2 ...
        }
    }
}

GetPixel.h

# ... existing code from Attempt 1 ...
void get_pixel_3x3(struct Colour3* pixels, int sx, int sy);

GetPixel.go

package x11util

func ScreenshotB(sx, sy, w, h int, filename string) {
    # ... existing code from Attempt 1 ...

    var i int
    var p C.Colour3 // why does this even work?
    for x := sx; x < w; x += 3 {
        for y := sy; y < h; y += 3 {
            // returns an array of 9 pixels
            C.get_pixel_3x3(&p, C.int(x), C.int(y))

            // unsafely convert to an array we can use
            pb := (*[9]C.Colour3)(unsafe.Pointer(&p))
            pa := pb[:] // seems to be required?

            // assign pixels from base (adding an offset 0-2) to print 3x3 blocks
            for xo := 0; xo < 3; xo++ {
                for yo := 0; yo < 3; yo++ {
                    # ... existing code from Attempt 1 ...
                }
            }

            i = 0
        }
    }

    # ... existing code from Attempt 1 ...
}

Attempt 4

Results in Segmentation Violation (SEGFAULT)

GetPixel.c

# ... existing code from Attempt 1 ...
void get_pixel_arbitrary(struct Colour3 *pixels, int sx, int sy, int w, int h)
{
    XColor c;
    for(int x=sx; x<sx+w; ++x)
    {
        for(int y=sy; y<sy+h; ++y)
        {
            # ... existing code from Attempt 3 ...
        }
    }
}

GetPixel.h

# ... existing code from Attempt 1 ...
void get_pixel_arbitrary(struct Colour3 *pixels, int sx, int sy, int w, int h);

GetPixel.go

package x11util

func ScreenshotC(sx, sy, w, h int, filename string) {
    # ... existing code from Attempt 1 ...

    var p C.Colour3 // I'm sure this is the culprit

    // returns an array of "all the screens"
    // 240x135x3x8 = 777600 (<768KB)
    C.get_pixel_arbitrary(&p, 0, 0, C.int(w), C.int(h)) // segfault here

    // unsafely convert to an array we can use
    pb := (*[1 << 30]C.Colour3)(unsafe.Pointer(&p)) // internet showed this magic 1<<30 is required because Golang won't make an array with an unknown length
    pa := pb[:w*h] // not sure if this is correct, but it doesn't matter yet, we don't get this far (segfault happens above.)

    // assign pixels from base (adding an offset 0-2) to print 3x3 blocks
    for x := 0; x < w; x++ {
        for y := 0; y < h; y++ {
            # ... existing code from Attempt 1 ...
        }
    }

    # ... existing code from Attempt 1 ...
}

Attempt 5

GetPixel.c

struct Colour3* get_pixel_arbitrary(int sx, int sy, int w, int h)
{
    XColor c;

    struct Colour3* pixels = (struct Colour3 *)malloc(w*h * sizeof(Colour3));
    struct Colour3* start = pixels;

    for(int x=sx; x<sx+w; ++x)
    {
        for(int y=sy; y<sy+h; ++y)
        {
            c.pixel = XGetPixel(im, x, y);
            XQueryColor(display, XDefaultColormap(display, XDefaultScreen(display)), &c);

            pixels->r = c.red / 256;
            pixels->g = c.green / 256;
            pixels->b = c.blue / 256;
            pixels++;
        }
    }

    return start;
}

GetPixel.go

    p := C.get_pixel_arbitrary(0, 0, C.int(w), C.int(h))

    // unsafely convert to an array we can use
    pb := (*[1 << 30]C.Colour3)(unsafe.Pointer(&p))
    pa := pb[: w*h : w*h] // magic :len:len notation shown in docs but not explained? (if [start:end] then [?:?:?])

    for x := 0; x < w; x++ {
        for y := 0; y < h; y++ {
            img.Set(x, y, color.RGBA{uint8(pa[i].r), uint8(pa[i].g), uint8(pa[i].b), 255})
            i++
        }
    }

    // assume I should be freeing in C instead of here?
    C.free(unsafe.Pointer(p))

Which produces a "stretched mess" which I then overflow on (now I have to assume I've done something wrong on the C side again, unless this is completely the wrong idea when it was mentioned I should allocate back in C?)

I am very willing to accept a guide with examples as the answer at this point, as there are things I'm missing surely -- however, when Googling (and even on GitHub) looking for examples of this, I couldn't find any. I would love to take the results of this thread and do just that :).

https://i.stack.imgur.com/Ht6Xq.jpg

AlbinoGeek
  • 33
  • 7
  • 2
    "this code is magic, have no idea how it works" not a good idea to have in your code, especially since it's probably incorrect. You really don't ever need to use `reflect.SliceHeader`, unsafe array conversions should use arrays, and if the length is static, it's even easier. – JimB Apr 09 '20 at 13:46
  • 1
    Can't you just fetch the entire image in a single call? Grabbing a 3x3 area probably isn't much of an improvement, and then you have to deal with the awkward handling of partial blocks. `img.Set` is probably the next slowest piece. If you want to fill that in with any efficiency, you need to access the `img.Pix` slice directly. – JimB Apr 09 '20 at 13:58
  • 1
    FWIW while you likely don't need it at all, your unsafe conversion should look something like: `ps := *(*[9]C.Colour3)(unsafe.Pointer(p))` (yes, you were also accessing the wrong memory) – JimB Apr 09 '20 at 14:02
  • Updated the question to include a new **Attempt 3 and 4**. 3 works for areas 3x3 or 8x8 etc, arbitrary hard-coded numbers. However, an attempt to pass "all the data" didn't work whatsoever. – AlbinoGeek Apr 09 '20 at 14:56
  • The last examples you're not leaving the allocation to Go, you're simply not allocating anything but a single `C.Colour3`. Don't try to pass Go memory to C until you know exactly what you're doing, as it's exceptionally unsafe with concurrent GC. Doesn't xlib have a `GetImage` api or similar? Your original problem is that the overhead here is going to be the number cgo calls, so you want to do it in as few as possible, preferably 1. All the subsequent problems seems to be misunderstanding of pointers and arrays. – JimB Apr 09 '20 at 15:11
  • @JimB Agreed, and sadly not quite. X11/xlib (or XCB) has `GetImage`, which I do use, and it has a `->data` that can be manually decoded to be sent over, but I have no understanding (where are concrete examples?) of allocating and passing a C array to Golang? -- if that's what I need to do. – AlbinoGeek Apr 09 '20 at 15:23
  • 1
    `#ifndef __GETPIXEL_` looks very professional. It is not: preprocessor symbols with one or two leading underscores followed by an uppercase letter are reserved. – wildplasser Apr 09 '20 at 15:25
  • 1
    There isn't much to passing C arrays into go, as a C array is simply a pointer. There's only 2 ways to handle this, both listed here: https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices. – JimB Apr 09 '20 at 15:31
  • @JimB Missing from that example was the implementation of ` C.getTheArray()` that could make me understand the example and adapt it here (found that before posting.) – AlbinoGeek Apr 09 '20 at 15:31
  • @wildplasser Thank you for pointing this out. I had simply copied the convention from other code-bases I'd read in C. – AlbinoGeek Apr 09 '20 at 15:32
  • Thank you all for the great information so far! I have attached now an Attempt 5 based on what was pointed out about the code. – AlbinoGeek Apr 09 '20 at 15:44
  • you're still copying pieces code labeled as "magic" without knowing what they do, which is probably the root of your problems. Using unsafe removes any of the type safety checks, which is allowing you to pass in the address of the pointer itself (you should also slice the array in the same expression, the full slice notation is explained in the [language spec](https://golang.org/ref/spec#Slice_expressions)). Maybe remove the unsafe conversion of C memory for now and just use `GoBytes` until you get it working. – JimB Apr 09 '20 at 15:53
  • @JimB And while it says simply to `func C.GoBytes(cArray unsafe.Pointer, length C.int) []byte` this is hardly an example of how to use it. I can only assume that means I must try `ba := C.GoBytes(unsafe.Pointer(p), C.int(w*h))` but then this does not explain the format the data would come in as, or how I would go about decoding it. – AlbinoGeek Apr 09 '20 at 15:58
  • The format is exactly the same, it's just copying the raw bytes into a slice for you. I guess it's not that big a deal if you don't use it, and it could be confusing for you since you're working at the byte level. First try doing the conversion _exactly_ as shown in the official examples, meaning `(*[1 << 30]C.Colour3)(unsafe.Pointer(p))[:w*h:w*h]` – JimB Apr 09 '20 at 16:03
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/211302/discussion-between-albinogeek-and-jimb). – AlbinoGeek Apr 09 '20 at 16:04

1 Answers1

2

Most of the failures here centered on using unsafe.Pointer(&p) for the returned C array. Since a C array is a pointer, p is already of type *C.Colour3. Using &p is attempting to use the address of the p variable itself, which could be anywhere in memory.

The correct form of the conversion will look like:

pa := (*[1 << 30]C.Colour3)(unsafe.Pointer(p))[:w*h:w*h]

See also What does (*[1 << 30]C.YourType) do exactly in CGo?

JimB
  • 104,193
  • 13
  • 262
  • 255
  • Thank you, it basically all boiled down to that (after picking a method of passing said pointer back to Golang.) Going to make a GitHub on different cgo dilemna encountered as a result of this Q. – AlbinoGeek Apr 10 '20 at 07:00