Closed donbing closed 2 years ago
looks good @aaronr8684
@donbing Sweet! Thanks for your help with this!
@dr-boehmerie, if you have some time, do you want to test this solution with your yellow screen?
@robweber what are your thoughts? I'm not saying that the solution above is the best, but it at least seems to work with all screen methods. I'm happy to discuss how you'd like to integrate this into the project before submitting a PR and also sitting on it a bit to see if we can come up with any additional ideas or improvements. Maybe set up a discussion item or we can just keep discussing on this issue. It would be a nice feature addition to offer the ability to change the default threshold (for the black image) and the two thresholds or maybe a single "band width" in the middle (for the color image). Maybe another optional line in the INI file. The dynamic idea might be a phase 2 kinda thing
As a summary of what I believe is happening here...
Using putpalette with a 'P' image mode assigns a custom palette to the image, but (and this is the important part) doesn't change the underlying image data. I think some of the waveshare drivers were able to handle this just fine and some couldn't. So changing the way the palette was setup would always break some of the screens. The solution is to pass a bw image to the get buffer methods of each driver file.
My solution (again, may not be the best solution), was to convert the newly filtered 'P' image to a greyscale image ('L'). Then taking that (in this case) 3-grey image and turning the grey and white pixels to white for the black image. Then for the red image, we take the grey pixels and turn them to black while turning the existing black pixels to white to avoid the red and black from overlapping on the ePaper. This essentially sends two bw images with no overlap to the display driver which is exactly what it expects. A 'L' mode image with just black and white data and a '1' mode image is basically the same so there doesn't seem to be any gain from doing a convert. Some (maybe all) of the drivers even do the convert to '1' inside the driver.
Maybe the end goal could be to have two options for the filter method. One could be the existing method of just grabbing the red (or yellow) from the image and then displaying it as it does now. Another filter method could be to do a weighted greyscale conversion that doesn't focus on converting the actual color represented by the screen but a range of colors close to the ePaper's third color to give a more dynamic (and more colorful) output. If you're curious about weighted greyscale, I found a cool article with a formula to convert from RGB to 265-greyscale - https://entropymine.com/imageworsener/grayscale/
For now, I think just patching the existing method is good enough and then we can play around with the more advanced methods if people have time.
I think we can all definitely agree that 8273217 was the original "source" of this particular issue; but only because of the subset of screens tested. The exhaustive testing on this confirms that with the current method some displays require an inverted b/w image while others don't. If we keep the code exactly that way it is, some displays will end up with black and white data inverted. If we revert the code then a different set of displays aren't going to work.
The putpalette()
solution, while simpler, doesn't seem like the best long term. We can code around the individual displays (inverted vs non-inverted) but as Waveshare updates their code it will be a constant battle to keep the list updated. It would appear that the method @aaronr8684 created yields correct results on both types of displays. Is that correct? If so that may be the best method to correct the issue "once and for all" (until it breaks again!). The threshold values should be some sort of sane default but I'd like it to be something the user can set as an ini
value as well. Since it effects the final image produced I think it's only fair the end user has control over this.
The more advanced, formulaic method is intriguing but could be a future PR. Ultimately offering a basic vs advanced method would be a fun enhancement. Does this direction sound like it will fix the issue for all displays that we know so far?
Yes, my original PR did fix some screens while breaking others and fortunately, it appears the new method will handle all tested screens correctly (with more to be tested going forward hopefully).
I'll go ahead and create the PR with some INI logic built in and defaults that will take into account both red and yellow screens. I settled on the numbers in the "full else block" test, by noting what values gave the correct output on the yellow AND red palette. I'll attach the outputs below for documentation.
@robweber Would you like me to update the documentation and examples as well or do you want to handle that part?
For the more advanced "option", I wasn't suggesting that be a part of this bug fix, just an interesting thought I had while testing all of this and a possible enhancement in the future.
Filtered Image Result
Red palette:
Yellow Palette:
Result of the point method on img_color
with values of 18-235:
Red:
Yellow:
I can update the documentation, that shouldn't be an issue. It will help me keep it all straight to really comb through it when complete anyway. In the PR could you reference any tested devices so I have them all in one spot for marking up the tested section of the README?
I'll add a "help wanted" enhancement for the more advanced filter should anyone have some spare time on their hands in the future!
I think there might be some misunderstanding of how this works.
First, the image is quantized using a three-color palette: white, black, red or white, black, yellow, in that order. Then, by changing the palette index of the color we want to black and the rest to white, we can extract pixels that are that color.
Palette Index | 0 | 1 | 2 |
---|---|---|---|
image |
White | Black | Red |
img_black |
White | Black | White |
img_red |
White | White | Black |
Before you go making it more complicated, can you please try both the suggestions in https://github.com/robweber/omni-epd/issues/58#issuecomment-1056336702, i.e, change https://github.com/robweber/omni-epd/blob/00f77b5d7094409eb46376de77f6655b5042abec/src/omni_epd/displays/waveshare_display.py#L209-L219 to this:
# apply the color filter to get a 3 color image
image = self._filterImage(image)
# separate out black from the other color
img_black = image.copy()
img_black.putpalette((255, 255, 255, 0, 0, 0, 255, 255, 255) + (0, 0, 0)*253)
img_color = image.copy()
img_color.putpalette((255, 255, 255, 255, 255, 255, 0, 0, 0) + (0, 0, 0)*253)
self._device.display(self._device.getbuffer(img_black.convert("RGB")), self._device.getbuffer(img_color.convert("RGB")))
I don't have any 3 color screens, otherwise I'd try it myself.
@missionfloyd Yes, I understand. The reason for my original PR (and commit 8273217) was because my WaveShare (7in5b_V2) was not working as expected with the existing code. Here is the output:
This prompted my PR, thinking (mistakenly) that ALL tri-color screens were broken. When the PR was merged, we discovered that while it had fixed my screen, it had also broken a few others (e.g. 5in83c and later the 2in7b_V2). Before this issue was raised, I had assumed that maybe the "old" or "yellow" method was broken and that the majority of screens would still work. After this issue was raised, I did some deeper digging into the code to figure out why the examples of all screens worked, but not the above code. This led me to my understanding of the 'P' mode in pillow remapping the data to color representation, but NOT the data itself. So to fix this, you have to make sure that the image is correctly representing black as black and white as white in the image data. Then each driver can modify the image to a buffer of values specific to each screen.
What's that the output of?
Your code and it's the same as what I saw originally
Waveshare's test images are indexed: black, then white. We've been using white, black. could the order of the palette be the problem? It shouldn't matter, all images get converted to 1-bit by the driver.
It doesn't make a whole lot of sense. we have the two images, the pixels that should be on are black in both, there has to be some format or palette or something they can be converted to that'll play nice with the drivers.
All I know is that the old code (and your test code) didn't work on at least my screen. Fixing it to work with mine broke others. I, and others, spent the time to test and come up with a solution that works with all screens tested so far and doesn't add any dependencies to the code. If you want to come up with a better solution that works on all of the tri-color screens, be my guest.
EDIT - I reread this and realized it came across as snarky. That wasn't my intention. I'm really open to other suggestions, or at least reasons why the solution I proposed isn't a good solution.
I'm really impressed by the level of detail going in to understanding how this is all working. Without a display to test it on myself I'm taking a face value what the output of the tests are showing. @missionfloyd's description of the current method for extracting the colors mirrors my understanding of how this was supposed to work. That being said it's obviously not for reasons I can't decipher.
I can't help but think part of the reason is how Waveshare has chosen to handle how it expects the images to be presented (either b/w or w/b depending on the device) but that's speculation on my part.
Yea, all I can say is that all of the waveshare devices accept a bw bmp file to the display method for the black and red images (through getbuffer). As we've discussed before, it sucks that everyone is handled differently and it appears that the 7in5b_V2 method will be the preferred direction because it uses the new send_data2 method which is considerably faster for updates.
All that to say, the test files provided by ws all work as expected across the different devices and the omni display method does not, so there seems to be a disconnect in what ws is expecting vs what omni is providing. I think (I don't know) that the it comes down to omni supplying a palettized image format when the ws drivers expect a non-palettized format. Sometimes it works and sometimes it doesn't. The two directions I think would work is 1) we can adjust the logic based on the screen type (which requires every screen to be actually tested) or 2) we can just provide a non-palettized image and assume it will probably work across all of the tri-color screens (like the supplied bmp files do). That takes the logic of how to represent the image away from omni and onto the ws drivers. It also would likely prevent code breaking changes if the underlying logic is changed in the drivers.
If giving it a grayscale image fixes it, then converting to grayscale might be all that's needed:
self._device.display(self._device.getbuffer(img_black.convert("L")), self._device.getbuffer(img_color.convert("L")))
Hi, I've got a
waveshare_epd.epd2in7b_V2
that i'm trying to show a picture on.when i configure the device for
red
mode using an ini file all I get is red pixels on the display, should it also display black?e.g: source image:
resized and converted to RBG:
displayed:
![rendered](https://user-images.githubusercontent.com/413860/155631238-e67ae672-00a2-4d24-a4a7-47de91432731.jpg)