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

Auto bitmap sizing from Metadata #34

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nullstalgia
Copy link
Contributor

This does work so far, but needs the following things if you feel this is worthy.

  1. More testing
  2. Clean up
  3. Reflect size in UI automatically

For issue #33

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

Love the concept -- will definitely be an improvement in user experience.

Have a couple of high level comments:

  1. Presently, metadata is optional and only used for UI hinting. This change essentially makes it required. We should either:

    A. Have some sane fallback behavior (like treat it as the nearest square). As is, looks like initial width/height will get initialized to randomized due to non-initialized variables in the absence of metadata. (this one makes the most sense to me)

    B. Make metadata required. Have the API throw an error if it's not provided, and update documentation to indicate how to provide it (it's a little bit tricky to do right, actually).

  2. Definitely want to adjust the UI to reflect this change. Right now, looks like the width/height form fields will still be there. We should nuke them from schema.js.

I have a handful of comments on stuff like formatting, style and performance. Happy to leave these in a review if you want to make changes yourself, or I can make the changes directly -- let me know what you prefer.

@nullstalgia
Copy link
Contributor Author

  1. I was under the impression it was required because it was always generated upon saving it in the editor. While I'm not against some sane fallback behavior and I'm not sure the format of the bin files... I'm not certain how you can calculate the nearest square.

  2. Agreed. I took it out at first but put it back in because I wasn't sure if it would break the UIs preview without it.

And when it comes to comments, feel free. It's your repo, I'm just someone who's a little too happy to help :p

And styling is a personal point of contention because I usually format with 4 spaces (I can work with 2, don't worry). But I use VSCode's C++ formatter (I think) and pushing a commit with that formatting would cause too many changed lines for no functional change.

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

And when it comes to comments, feel free. It's your repo, I'm just someone who's a little too happy to help :p

Super happy to have you helping out. It's been great.

Only asking your preference here because I want to make it as comfortable of a contribution environment as I can. :)

I was under the impression it was required because it was always generated upon saving it in the editor. While I'm not against some sane fallback behavior and I'm not sure the format of the bin files... I'm not certain how you can calculate the nearest square.

The UI always populates it, but the API doesn't enforce that it exists (because at the time, it was only useful for the UI).

The bin files are straight-up bitmaps (the data structure, not the file format). The number of pixels in the image is fileSize / 8, so the nearest square dimension is sqrt(fileSize / 8).

And styling is a personal point of contention because I usually format with 4 spaces (I can work with 2, don't worry). But I use VSCode's C++ formatter (I think) and pushing a commit with that formatting would cause too many changed lines for no functional change.

Haha - I only care about formatting for consistency. If it helps, I use clang-format (there's a VSCode plugin), and the rules are defined in ./.clang-format.

@nullstalgia
Copy link
Contributor Author

Super happy to have you helping out. It's been great.

Glad to hear it! It's a fun project that I see many uses for. Especially since it just becomes much easier to make projects that can use an e-paper display and is okay with just reading from MQTT. Much more efficient than getting a Kindle, jailbreaking, getting a web browser page set up to not go to sleep, get a proper JS interface to read, scream at JQuery not working on old kindles, cry w̵̨̨̻͙͎̰̹̱̖̲̥̦̫͇͊̈̌̐̃̑͒̀̀̈́̚͜h̴̡̜͈̅̃̇̎̉̆̀͗͜ÿ̵̦͑̽̓̃̓͌̍͠ ̷̛͔̗͙̥̲̫̠̟̭͑͐̇̾͊̅̔̀̽̿̊͑͂͠ͅa̸̢͍̘̤̻̟̝̜͖̰͎͍͆̔̈́̉́̃̒̐͝ṁ̵͔̘͉̩̹͓͎̲̞̬̟̳͖͑ą̶̹̲͕̜̥͊̌̾̆̀̆̃̊͆͗͂̿̀̀͘z̵̨̤̗̼̟͇̙̎̄ͅò̶̼̱̙̟͎̬͔̲̳̠̲̉́̉̆̐̽͆̀͌́̇̚͠n̴̨̨̗̲̙̣̫̯̜͌̀͒̉ͅ ̸͇͇̳̺̎̌ẃ̵͕̝̞̙͖͈̪̬͚̪̆̎͒̋̆̂̚͝h̸̢͓̯̦̾͜ÿ̶̧̨̡̦͉͈̪͍̮̗̤͍͔̙͎̎̾̈́̎̾̓͋̐̕͝.

So I'm happy to help!

The bin files are straight-up bitmaps (the data structure, not the file format). The number of pixels in the image is fileSize / 8, so the nearest square dimension is sqrt(fileSize / 8).

While I don't doubt you and I haven't gone too much in-depth into it, I'm just wondering about more rectangular images! Like a 1:2 aspect ratio.

Formatting

oh shiz you're right, I didn't see that. Hopefully I can wrangle VSCode to take it, then. Linux-VSCode implementation of certain linters/formatters refuse to be discovered. Doubly so with PlatformIO in the mix.

lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
lib/Display/BitmapRegion.cpp Outdated Show resolved Hide resolved
Serial.println(F("WARN - tried to get dimensions from bitmap that doesn't exist: "));
Serial.println(bitmapName);
Serial.println(metadata_filename);
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calculate default based on bitmap file size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we strike this or keep it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see three options:

  1. Calculate the default somewhere (was thinking this was the appropriate place)
  2. Calculate a default on the API side and generate a default metadata file so that we can treat the existence of a metadata file as an invariant (in which case what you have here is fine)
  3. Refuse to save bitmaps that don't have metadata files supplied (same deal -- we get the invariant)

I was originally proposing (1), but (2) hadn't occurred to me until now, and I think I like that better. Open to whatever makes most sense to you, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~~2, I feel is best. I think it should save the bitmap even if there isn't a supplied W and H with a default set by us ~~

Actually, no. 3. Since if you use the resizer in the bitmap editor, it will screw up the already supplied bitmap.

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI always submits the dimensions of the image every time it's saved, so that shouldn't be a problem.

Either (2) or (3) seem fine.

lib/Display/DisplayTemplateDriver.cpp Outdated Show resolved Hide resolved
lib/Display/DisplayTemplateDriver.cpp Outdated Show resolved Hide resolved
@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

Just left the fiddly suggestions -- I can make those changes myself if you prefer.

re: non-square images, you're right, there's no way to do this without metadata. Just meaning to suggest a default behavior that makes sense as often as possible. Option (B) is the only alternative I see. That's fine, just makes the API more rigid.

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

oh, and on the UI front -- it's possible that parts of the preview canvas is relying on the width/height in the definitions. If you're getting strange behavior there (errors or distorted images), that's probably why. Happy to help out with that, just lmk.

@nullstalgia
Copy link
Contributor Author

A more rigid API isn't always bad. It just depends.

And having it at least generated automatically when you supply the file, W, and H, it shouldn't be an issue (in my eyes. I could be wrong).

UI

Yeah, I was having that issue from minute one. It was fixed by refreshing/reopening the template editor. Having it re-render it upon the bitmap change should be easy enough?

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

lol, of course. Just meant that in the way of expressing a cost.

Uploading a bitmap with curl, for example, would be something like this:

curl -F 'metadata.json={"width":50,"height":50}' -F '[email protected]' epaper-display/api/v1/bitmaps

It works, it's just awkward.

Yeah, I was having that issue from minute one. It was fixed by refreshing/reopening the template editor. Having it re-render it upon the bitmap change should be easy enough?

Yeah, shouldn't be a problem. There's a fair amount of plumbing involved to make this happen, but should be mostly straightforward. Let me know if you want a hand :)

@nullstalgia
Copy link
Contributor Author

Note

You will want to remake your examples after this, likely.

Also, how would you do the upload_bitmaps script when it comes to their sizes? unless they are all one size.

What if we scaled it on the ESP before displaying?

Size won't matter as much, you can use the same picture multiple times without wasting SPIFFs space, and the issue with the upload_bitmaps will be lessened (but it will still be there)

lib/HTTP/EpaperWebServer.cpp Outdated Show resolved Hide resolved
@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

You will want to remake your examples after this, likely.

Also, how would you do the upload_bitmaps script when it comes to their sizes? unless they are all one size.

Great points.

If I were writing the script for myself, I'd probably use identify from ImageMatick to extract the width and height like so:

identify -format '{"width":%w,"height":%h}' "${file}" \
  | curl -s "${server}/bitmaps" -F filename=@${file} -F "metadata.json=@-"

That script could use some love, though. Pretty raw as-is. (also not clear to me it's terribly useful. if batch uploads are worthwhile, should add that functionality to the UI).

Size won't matter as much, you can use the same picture multiple times without wasting SPIFFs space, and the issue with the upload_bitmaps will be lessened (but it will still be there)

This is a cool idea, but scaling images properly is surprisingly challenging. I even avoid it in the UI by punting it to the browser (by either up- or down-scaling the image in an img tag and then accessing it pixel-by-pixel in javascript).

I could definitely be convinced otherwise, but I'd prefer to keep bitmaps fixed size. Both to avoid the additional complexity, and because doing it well is gonna be pretty hard.

@nullstalgia
Copy link
Contributor Author

Okay so I had a similar issue with scaling a while ago when I was creating an ESP32-connected Thermal Printer (receipt printer). The QR codes I would generate would be too small, and I also wanted it to be padded to the center of the receipt.

I have to run somewhere quickly, but I want to send this to you before I go so you can keep it in mind and maybe think about using it.

CPP

#include "Arduino.h"
#include "qrcode.h"
#include "QRCodeStream.h"
// Written by Arix Zajicek
int QRCodeStream::available()
{
    // For simplicity, pretend the bytes come one at a time.
    if (done) return 0;
    else return 1;
}

int QRCodeStream::peek()
{
    
}

void QRCodeStream::begin()
{
    // Set the starting index 
    byteIndex = 0;
    scaledQrSize = scale * qrcode.size;
    lPadding = (384 - scaledQrSize) / 2;
    
    bmpWidth = scaledQrSize + lPadding;
    if (bmpWidth % 8 != 0){
        bmpWidth += 8 - (bmpWidth % 8);
    }
}

int QRCodeStream::debug(int which){
    return 1;
}


int QRCodeStream::read()
{
    if (byteIndex > bmpWidth / 8 * scale * qrcode.size) return -1;
    
    int data = 0;
    
    int row = byteIndex / (bmpWidth / 8);
    
    for (int col = (byteIndex * 8) % bmpWidth; col < (byteIndex * 8) % bmpWidth + 8; col++) {
        bool pixel = false;
        
        if (col >= lPadding & col <= lPadding + scale * qrcode.size) {
            pixel = qrcode_getModule(&qrcode, (col - lPadding) / scale, row / scale);
        }
        
        data = data | (pixel << (7 - (col % 8)));
    }
    
    byteIndex++;
    return data;
}

Header

#include "Arduino.h"
#include "qrcode.h"
// Stream a scaled QRCode one pixel at a time.
class QRCodeStream : public Stream
{
  public:
    QRCodeStream(QRCode &code, uint8_t scale = 1)
      : qrcode(code), scale(scale) {}

    // Output: writing to this stream is a no-op.
    size_t write(uint8_t) {}
    void flush() {}

    // Input delivers the QRCode pixels.
    int available();
    int peek();
    void begin();
    int debug(int which);
    int read();
    
    int bmpWidth;
    int scaledQrSize;
  private:
  
    QRCode &qrcode;
    uint8_t scale;
    
    int lPadding;
    int byteIndex;
    
    bool done = false;
};

This is written by my Cpp Wizard friend, not me.

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

Yeah, naive up-scaling is pretty straightforward (it's called nearest-neighbor interpolation): each pixel in the new image is mapped to a single pixel in the source, and its value is determined solely from it. That's all that's going on in that code sample.

But:

  1. Nearest-neighbor up-scaling works fine for really blocky things like QR codes (for QR codes in particular, it's actually exactly what you want), but is usually not what you want with images with more curves.
  2. Downscaling is harder.

It's not that I don't think I can do it -- I definitely can. It even sounds fun. I just don't want to venture too far outside of the core value of the project :)

@nullstalgia
Copy link
Contributor Author

  1. I could be wrong, but I'm not sure how much of an issue it would be if it was nearest-neighbor. Considering how small these things usually are, on a small screen, it probably wouldn't be that noticable.

(I could be totally wrong, however.)

How does the scaler in the bitmap editor work?

  1. I was thinking it would be only scaling up, a la the font size in the template editor.

  2. When it comes to the UI glitches, I couldn't even get the enumNames right. I think that's more in your ballpark 😂

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

I'll keep it in the back of my head; cloud definitely be convinced that it's a good idea.

To give you an idea of what I'm getting at, here's a 64x64 resized from a 32x32 --

image

vs the native 64x64 --

image

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

I dodge the problem in the UI by making the browser do the work (browsers obviously have to be able to scale images, and they do it really well).

@nullstalgia
Copy link
Contributor Author

I look at that and think "Well just make the lines thinner, dummy," but that's not a reasonable solution..

This is a topic for another time, perhaps.

Do you feel it's still worthwhile to add this automatic size-detection or no?

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

Yeah, it's a hard problem to solve well generally. Agreed -- let's discuss elsewhere if we want to continue.

Automatic size detection definitely worth it. This is a good change.

Think we've got (A) API hardening, and (B) making sure the UI works with the change left. I'm happy to help out with either or both of those, but will leave it to you unless you let me know what you'd like help with :)

@nullstalgia
Copy link
Contributor Author

I might take a crack at the API, but the UI is something totally esoteric to me. If you could lend a hand there, that would be much appreciated.

I haven't delved into the API section of the project yet, but it shouldn't be that bad, right?

@sidoh
Copy link
Owner

sidoh commented Apr 5, 2020

Sure, sounds good. I'm playing around with a wacky idea right now (adding a Lua scripting engine), so might be a bit before I get to it.

Nah, I expect the API stuff to be mostly straightforward. Let me know if you'd like some pointers. :)

@nullstalgia
Copy link
Contributor Author

Lua, huh? Haven't heard that since The Bodge and Garry's Mod.

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 this pull request may close these issues.

2 participants