Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove canvas dependency #10

Closed
Julusian opened this issue Jun 8, 2022 · 5 comments · Fixed by #14
Closed

Remove canvas dependency #10

Julusian opened this issue Jun 8, 2022 · 5 comments · Fixed by #14

Comments

@Julusian
Copy link
Contributor

Julusian commented Jun 8, 2022

I just received a loupedeck live and was expecting to have to finish/rework https://github.com/bitfocus/loupedeck-ct to be able to use it. Finding a library that is working and with a reasonable api was a nice surprise!

One problem I do have however, is that this library depends on canvas. This is a bit of a dealbreaker for me, as I already have sharp being used heavily in the projects, and canvas are known to not be compatible with each other https://sharp.pixelplumbing.com/install#canvas-and-windows.

Are you open to removing the dependency on canvas?

In node-elgato-stream-deck it is expecting buffers of the correct dimensions to be supplied, and then does some manual buffer manipulation to do any colour conversion or image flipping. Doing it in nodejs like this doesnt have much of a performance impact from what I have found. (Now I am wondering if I could make it faster.. perhaps a perfect use case for wasm?)
This lets users of the library bring whatever image generation technology they want. I know of one project which uses a custom button drawing class, with a partner project which receives pre-generated buffers over tcp, and others which are using sharp to render an svg to a buffer. I think I have seen some users who are using 'inefficient' methods because they only need a few images that never change, so a native library is more effort than benefit.

@foxxyz
Copy link
Owner

foxxyz commented Jun 9, 2022

Thanks for the kind words!

I too, wish that canvas on Windows would no longer rely on a 10 year old GTK bundle and support modern versions of cairo, pango, etc. (Relevant issue here: Automattic/node-canvas#1768). This would solve the sharp conflict, but I do understand the canvas author justifying the status quo by saying "Well, it still works".

I'd be open to using a different library, but as far as I know canvas is the only library that implements most of (and adheres to) the standard Canvas API, which makes it easy to write portable graphics routines that can be tested in a browser and then run on a Loupedeck. sharp has some great image manipulation tools (I use it for a different project, also), but it does not implement this spec - so programmatically drawing images becomes a lot more difficult.

If you disagree, I'm happy to hear what your suggestion would be - specifically I'd be curious how you'd implement some of the examples.

@Julusian
Copy link
Contributor Author

Julusian commented Jun 9, 2022

The ideal thing for me is to have a method like this:

async drawBuffer({ id, width, height, x = 0, y = 0, buffer, autoRefresh = true }) {
        const displayInfo = DISPLAYS[id]
        if (!width) width = displayInfo.width
        if (!height) height = displayInfo.height

        const pixelCount = width * height
        if (buffer.length !== pixelCount * 3) {
            throw new Error(`Incorrect buffer length ${buffer.length} expected ${pixelCount * 3}`)
        }

        const converted = Buffer.alloc(pixelCount * 2)
        for (let i = 0; i < pixelCount; i++) {
            const r = buffer.readUInt8(i * 3 + 0) >> 3
            const g = buffer.readUInt8(i * 3 + 1) >> 2
            const b = buffer.readUInt8(i * 3 + 2) >> 3

            converted.writeUint16LE((r << 11) + (g << 5) + b, i * 2)
        }

        // Header with x/y/w/h and display ID
        const header = Buffer.alloc(8)
        header.writeUInt16BE(x, 0)
        header.writeUInt16BE(y, 2)
        header.writeUInt16BE(width, 4)
        header.writeUInt16BE(height, 6)

        // Write to frame buffer
        await this.send(HEADERS.WRITE_FRAMEBUFF, Buffer.concat([displayInfo.id, header, converted]), { track: true })

        // Draw to display
        if (autoRefresh) await this.refresh(id)
    }

It takes in a pre-generated rgb buffer of the correct dimensions, and does the required pixel manipulation before writing it out.

While it makes the usage a lot less elegant, it can also be used with canvas manually. Something like:

const canvas = createCanvas(width, height)
const ctx = canvas.getContext('2d') // Might need to set it as rgb? Or modify drawBuffer to handle different input formats

// Do your drawing here

return myLoupedeck.drawBuffer({ id, width, height, x, y, buffer: canvas.toBuffer('raw'), autoRefresh })

The main downside being that it will be up to the callers to know what dimensions to create the canvas, but they could be exposed as properties on the class (This is what is done the elgato-stream-deck, as different models have different display sizes)

I actually implemented that method to allow me to do some testing yesteray. I was trying to draw an already generated rgb pixel buffer, but after some fighting with trying to figure out how to get that onto the canvas in the correct colour space, I gave up and wrote this alternate method.
The canvas being created as RGB16_565 was really complicating drawing a rgb pixel buffer. The only way I could find to convert the pixel buffer into someting for canvas was createImageData(). With the only way to draw that being putImageData. But it want handling conversion of the pixelFormat. I didnt try for too long, as it was starting to get really tedious, especially when bypassing the canvas entirely for this will be much simpler and more performant.

If you like I can throw together a branch with this actually implemented, so you can see it in action

@foxxyz
Copy link
Owner

foxxyz commented Jun 10, 2022

I'm happy with a compromise, if you're open to it:

I would support implementing the .drawBuffer() method (similar to what you proposed above) to allow users to send (plain RGB) buffers directly to the device. I would then adapt .drawCanvas() to use this method by added a convertToRGB16 option that can be set to either do the conversion or not.

It would also be nice if the .drawKey() and drawScreen() methods would allow a Buffer for the second argument. If the second arg is a callback, users could still treat the device as a canvas context. If the second arg is a Buffer, it would just check dimensions and write the Buffer to the device directly. But I'd certainly want to keep canvas drawing in the library.

@Julusian
Copy link
Contributor Author

I would support implementing the .drawBuffer() method (similar to what you proposed above) to allow users to send (plain RGB) buffers directly to the device. I would then adapt .drawCanvas() to use this method by added a convertToRGB16 option that can be set to either do the conversion or not.
It would also be nice if the .drawKey() and drawScreen() methods would allow a Buffer for the second argument.

That all sounds ideal for me. convertToRGB16 should probably be done as a pixelFormat: 'rgb' | 'rgb16', as some image processing libraries will only give rgba, or some buffers might be bgr. So an explicit pixelFormat like this will allow for adding more modes later. This is similar to what elgato-stream-deck does [https://github.com/Abrahamic-God/node-elgato-stream-deck/issues/61#issuecomment-600559991](motivated by a user issue)

I'm happy with a compromise, if you're open to it:
But I'd certainly want to keep canvas drawing in the library.

I would like to be able to say that is enough, but unfortunately the presence of the library is still problematic for me.
It will still be loaded into memory because of the require('canvas') call, which means this library still wont be compatible with applications using sharp. That could be avoided by only loading the library the first time it is needed, but Im not sure if that could be problematic as require is a blocking and sychronous operation.

But the big problem is that I expect this to be used on a raspberry pi a lot. Unfortunately as canvas doesnt have arm prebuilds, that means each machine will need to compile it locally.
For completeness I have given it a try. After installing the correct system libraries, it took my pi4 about a minute to compile canvas. For a library that will not be used that is quite a complexity cost.
This is a similar story to that issue I linked earlier, but with sharp being the painful library Abrahamic-God/node-elgato-stream-deck#61. Some users who wanted to avoid that pain forked the library just to remove the dependency https://www.npmjs.com/package/elgato-stream-deck-clean

@foxxyz
Copy link
Owner

foxxyz commented Jun 11, 2022

Thank you for testing the RasPi scenario, that's very useful! It strengthens the case for decoupling.

Here's a possible solution that will satisfy both of us... What if we made canvas an optional peer dependency?

That way:

  1. canvas would only be loaded (dynamically) if available
  2. If .drawCanvas() is called with canvas being installed, everything works as it currently does
  3. If .drawCanvas() is called without canvas being installed, a warning is emitted informing the user they need to install canvas to use that functionality

foxxyz added a commit that referenced this issue Jul 16, 2022
foxxyz added a commit that referenced this issue Jul 17, 2022
…directly (#14) (close #10)

* refactor: make `canvas` an optional peer dependency and accept raw RGB565 buffers in `drawBuffer`, #10
* test: add tests for buffer API
* ci: install canvas for peer dep tests on node < v16
* docs: add `drawBuffer` documentation
* docs: add note on running examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants