Im7 Add SetImageMask #256
Im7 Add SetImageMask #256
Conversation
|
Refs eventual fix: ImageMagick/ImageMagick#3566 |
|
@pfitzner did this work for you? |
|
Hello Justin,
Sorry I didn’t get back to you sooner.
I converted the project to modules and did a successful ‘go get’ on your branch, but I couldn’t get the import syntax to work. I have a friend who's an IBM gopher, and he wasn’t able to help. So, I’ve regressed to IM 6, and will wait until your branch is merged into master before upgrading to IM 7.
Thanks for the effort, but it was work that had to be done anyway.
Bob S.
PS. Go is mostly a pleasure to code in, except for the medieval error handling, lousy documentation, and Rob Pike’s ego.
… On Apr 24, 2021, at 10:18 PM, Justin Israel ***@***.***> wrote:
@pfitzner <https://github.com/pfitzner> did this work for you?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LP7BZWWIQPNFPRQZA73TKN3V7ANCNFSM43GUDZTA>.
|
|
@pfitzner, I would have thought it would work fine in module-mode via: Anyways, I added a quick test just to make sure it doesn't crash, and merged it to the im7 branch (which is the main branch for v3). Thanks for the general commentary on the Go language. I use Go in multiple production projects, but to be honest I only ended up becoming the maintainer of this project because at one point I was a user of it and no one else was maintaining it. So I just keep it going now and fix things for people when I have time. |
|
Justin,
The 'go get' works fine. It's just that you can't
import ***@***.***_SetImageMask ***@***.***_SetImageMask>"
When I left the branch off the import, the compiler thought I wanted master, even though the branch was in go.mod.
Thanks for the merge. Now I'll have to trash IM 6, and reinstall IM 7. But not before a complete Time Machine backup.
Thanks, you're a prince.
Bob S.
PS. Did I mention the lack of compiler optimization?
PPS. I'm getting too old for this.
… On Apr 24, 2021, at 11:49 PM, Justin Israel ***@***.***> wrote:
@pfitzner <https://github.com/pfitzner>, I would have thought it would work fine in module-mode via:
go get ***@***.***_SetImageMask
Anyways, I added a quick test just to make sure it doesn't crash, and merged it to the im7 branch (which is the main branch for v3).
Thanks for the general commentary on the Go language. I use Go in multiple production projects, but to be honest I only ended up becoming the maintainer of this project because at one point I was a user of it and no one else was maintaining it. So I just keep it going now and fix things for people when I have time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LPZQTAP3HLMWL4KKSXDTKOGLZANCNFSM43GUDZTA>.
|
|
Justin,
Now that you've rolled it into master, the module problem is obviated and so I converted my little project to non-module. I'm happy to report that it works as expected.
Now the only thing left is to understand how! What exactly are the behaviors of the read and write pixel mask types in the call? It's really poorly documented for a routine necessary for porting.
Thanks again,
Bob Schaaf
… On Apr 24, 2021, at 11:49 PM, Justin Israel ***@***.***> wrote:
@pfitzner <https://github.com/pfitzner>, I would have thought it would work fine in module-mode via:
go get ***@***.***_SetImageMask
Anyways, I added a quick test just to make sure it doesn't crash, and merged it to the im7 branch (which is the main branch for v3).
Thanks for the general commentary on the Go language. I use Go in multiple production projects, but to be honest I only ended up becoming the maintainer of this project because at one point I was a user of it and no one else was maintaining it. So I just keep it going now and fix things for people when I have time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LPZQTAP3HLMWL4KKSXDTKOGLZANCNFSM43GUDZTA>.
|
|
@pfitzner I actually have no idea how MagickSetImageMask works, beyond what the ImageMagick C API actually documents. https://imagemagick.org/api/magick-image.php#MagickSetImageMask We usually just copy across the doc string from the C API. And in this case they don't seem to explain the PixelMask enum. https://imagemagick.org/api/MagickCore/pixel_8h.html#a6e97344e98e8667f420da73ba326c25e So I am not sure if you are calling out these Go bindings as being poorly documented, or ImageMagick. If it's the latter, then I agree it is insufficiently documented to easily know what that parameter controls. One might need to either Google for an example of using that api call, or ask for clarification on the ImageMagick forum? |
|
Justin,
No, I really can't recall having to go further than the Go docs for clarification.
Rummaging around in the core API, I did find another routine that hasn't made the port, but probably isn't worth it,
extern MagickExport Image
*ConnectedComponentsImage(const Image *,const size_t,ExceptionInfo *);
I rolled my own, based on the readily available algorithm, which works with B&W masks and whose output is far easier to use. It's tolerbly fast in Go, and wickedly so in Rust.
I'd learn to use CGO if I knew that it's overhead wouldn't eat up the speed advantage.
The files for MagickConnectedComponentsImage <https://imagemagick.org/api/MagickWand/magick-image_8c.html#ae748542bb85b7f4dd24b6a48e156ead0> are vision.h/vision.c. The routine goes on forever! ☠️☠️☠️
Thanks again for your help. I will eventually ask for clarification on StackOverflow, but looking at examples of composite modes is like taking an IQ test.
Bob S.
… On Apr 26, 2021, at 6:44 AM, Justin Israel ***@***.***> wrote:
So I am not sure if you are calling out these Go bindings as being poorly documented, or ImageMagick. If it's the latter, then I agree it is insufficiently documented to easily know what that parameter controls. One might need to either Google for an example of using that api call, or ask for clarification on the ImageMagick forum?
|
|
Try here: https://github.com/ImageMagick/ImageMagick/discussions Don't forget, this project is just a binding. So it's not meant to be authoritative on how to use the project it binds. We just expose ImageMagick through cgo and ensure that our layer doesn't have bugs or leaks. If it was not a binding, then I would expect it to adequately document everything. |
|
Hello Justin,
I hate to be a pest, but I've run into a dead end with imagick. On a Mac Mini M1 with 16 GB, calls to wand.ReadImage, followed by wand.ExportImagePixels in a tight loop crashes, when it hits cgocall.go:154 (entersyscall).
This works perfectly in rust, since all deallocations are determinate, but here I think that wand.Destroy is somehow borking the GC thread. What do I know? It also works fine in v6 on Mac amd64. It also works with small images.
My question is, do you have access to an M1 Mac to test a very brief test case?
Tears in my eyes,
Bob Schaaf
// stresstest.go
package main
import (
"fmt"
// "gopkg.in/gographics/imagick.v2/imagick"
"gopkg.in/gographics/imagick.v3/imagick"
)
func main() {
var my_grayscale = "/Users/rwschaaf/Pictures/Cabinet_pics/outlines/33540_r.png"
var err error
imagick.Initialize()
defer imagick.Terminate()
for i := 1; i < 5001; i++ {
outline := imagick.NewMagickWand()
err = outline.ReadImage(my_grayscale)
if err != nil {
panic(err)
}
width := outline.GetImageWidth()
height := outline.GetImageHeight()
pix, err := outline.ExportImagePixels(0, 0, width, height, "I", imagick.PIXEL_CHAR)
if err != nil {
panic(err)
}
pixels, ok := pix.([]byte)
if !ok {
panic("Pixels not extracted")
}
fmt.Printf("%d: %d pixels.\n", i, len(pixels))
outline.Destroy()
}
fmt.Println("Done")
}
… On Apr 26, 2021, at 2:34 PM, Justin Israel ***@***.***> wrote:
Try here: https://github.com/ImageMagick/ImageMagick/discussions <https://github.com/ImageMagick/ImageMagick/discussions>
Don't forget, this project is just a binding. So it's not meant to be authoritative on how to use the project it binds. We just expose ImageMagick through cgo and ensure that our layer doesn't have bugs or leaks. If it was not a binding, then I would expect it to adequately document everything.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LP6DGEJOXLOGMQIKGUTTKWW37ANCNFSM43GUDZTA>.
|
|
@pfitzner unfortunately no I don't have a Mac M1 to test this against. I was only able to run your code on ImageMagick7 with Ubuntu 18.04. And as you said about other architectures, it didn't crash or show any kind of steady memory increase. What size images are you testing with? I tried between 3-7 MB images. I recall Mac M1 arch being something that has newly been added as support to the Go tooling, so maybe there is some kind of bug if it only happens on this architecture? You aren't using Goroutines in this example so it shouldn't be trying to share anything across threads. But for completeness you could try adding this at the top of your main? https://golang.org/pkg/runtime/#LockOSThread func main() {
runtime.LockOSThread()
defer runtime.UnlockOSThread()That forces the current goroutine to be locked to the current OS thread. Aside from that, you might want to consider posting this reproduction as a bug report for Go, or to the golang-nuts mailing list? |
|
What a man! What a man!
Debugging by waving your hands in the air. And it worked!
Tested with a 5.1 MB, 1556 x 1593 RGB image, and it chugs through 5000 iterations with a couple of breaks for GC. The grayscales are much smaller, though.
I suppose this is bad Go behavior, and should be investigated. I'll post something to go-nuts and hope it percolates up to the go team. If not, I'll have to bite the bullet and contact them myself.
Thanks, as always,
Bob Schaaf
… On Jun 9, 2021, at 6:53 PM, Justin Israel ***@***.***> wrote:
@pfitzner <https://github.com/pfitzner> unfortunately no I don't have a Mac M1 to test this against. I was only able to run your code on ImageMagick7 with Ubuntu 18.04. And as you said about other architectures, it didn't crash or show any kind of steady memory increase. What size images are you testing with? I tried between 3-7 MB images.
I recall Mac M1 arch being something that has newly been added as support to the Go tooling, so maybe there is some kind of bug if it only happens on this architecture?
You aren't using Goroutines in this example so it shouldn't be trying to share anything across threads. But for completeness you could try adding this at the top of your main? https://golang.org/pkg/runtime/#LockOSThread <https://golang.org/pkg/runtime/#LockOSThread>
func main() {
runtime.LockOSThread()
defer runtime.UnlockOSThread()
That forces the current goroutine to be locked to the current OS thread. Aside from that, you might want to consider posting this reproduction as a bug report for Go, or to the golang-nuts mailing list?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LP5POB6H65RY6VGHIT3TR7WE5ANCNFSM43GUDZTA>.
|
|
@pfitzner thats great to hear that my suggestion helped isolate the fix! I will watch for your thread to see if they identify it as a Mac M1 arch specific cgo bug |
|
Hello Justin,
Thanks for backing me up on golang-nuts. It was a major blunder forgetting to mention the successful tests on other architectures.
It looks like any call or sequence of calls into IM that might churn memory on the C side should be bracketed by a lock. Maybe even simpIe getters and setters. 🤯
As it's a known problem with Go and foreign code, these guardrails ought to go into the API surrounding the actual cgo call. In any case, a warning aught to be put into the docs recommending thread locking, and hope that the Go team has optimized the hell out of the call.
Chalk another one up to Go's downside.
Bob S.
… On Jun 9, 2021, at 9:19 PM, Justin Israel ***@***.***> wrote:
@pfitzner <https://github.com/pfitzner> thats great to hear that my suggestion helped isolate the fix! I will watch for your thread to see if they identify it as a Mac M1 arch specific cgo bug
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LPZ64C7JYEM5MWM57VTTSAHJZANCNFSM43GUDZTA>.
|
Seeing as the problem only cropped up under a specific architecture, we probably don't want to try and put too many fixes in place for general use. A documentation note is probably a good idea, to indicate when this situation can occur and what a recommended fix could be.
I agree. We could note that we have detected this problem in Mac M1 targets and offer an example of which scope one may want to add LockOSThread
Snarky comments about Go are not necessary here. It's obviously an issue with cgo on a specific target. Cgo is not Go. It is just a mechanism to translate calls. Rust does not use an m:n scheduler like Go, which makes Rust calls map more directly to OS threads (amongst other calling convention differences) . There isn't a problem with Go here. Just the compatibility of a cgo compilation target. |
|
Sorry for the snark. Just my frustration due to lack of experience.
You're right about massing with the API. Perhaps it's best just to note the problematic calls and warn about them. My use of Imagemagick is probably atypical, and doesn't cover a whole lot of the calls. It's more computational, for feature extraction (connected component labeling). Still, if the future MacBook Pros turn out to be a preferred development platform, more unexpected strangeness with native threads might arise.
Again, thanks for your charitable work, and sorry for the gut punch. It was unintended.
Bob Schaaf
… On Jun 12, 2021, at 5:50 PM, Justin Israel ***@***.***> wrote:
It looks like any call or sequence of calls into IM that might churn memory on the C side should be bracketed by a lock. Maybe even simpIe getters and setters. 🤯
As it's a known problem with Go and foreign code, these guardrails ought to go into the API surrounding the actual cgo call.
Seeing as the problem only cropped up under a specific architecture, we probably don't want to try and put too many fixes in place for general use. A documentation note is probably a good idea, to indicate when this situation can occur and what a recommended fix could be.
I don't think adding a LockOSThread around a single cgo call will fix this, seeing as the problem might occur in combination of acquiring and freeing C memory from ImageMagick? We wouldn't have api control over the lifetime to keep the acquisition and destruction within the same OS thread. However if it really does happen only within a single cgo call then maybe it could be fixed. But I interpreted your report as being about the entire lifetime. ImageMagick C API has its own specific calls to handle the memory, which we call into. And it could be possible they are relying on threadlocal storage.
In any case, a warning aught to be put into the docs recommending thread locking,
I agree. We could note that we have detected this problem in Mac M1 targets and offer an example of which scope one may want to add LockOSThread
Chalk another one up to Go's downside.
Snarky comments about Go are not necessary here. It's obviously an issue with cgo on a specific target. Cgo is not Go. It is just a mechanism to translate calls. Rust does not use an m:n scheduler like Go, which makes Rust calls map more directly to OS threads (amongst other calling convention differences) . There isn't a problem with Go here. Just the compatibility of a cgo compilation target.
Try to stay positive. It ensures others will want to offer help for free, even when the project they are helping someone with is not a project they initially created, or even use anymore 😉
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH5LP4F2GW6OECX3OHNKOTTSPJBLANCNFSM43GUDZTA>.
|
Fixes #245