free drawing: visible dots with opacity <1 and lag issues

[AndrewJDR]

Version

2.2.3

Test Case

http://jsfiddle.net/uk7jvr6o/12/

Steps to reproduce

Start drawing.

Expected Behavior

No dots along the line, and no performance/lag issues.

Actual Behavior

Dots along the line, and performance/lag issues.

---

This particular pull request - #4743 - caused two issues that I can notice:

1. If using a brush color with an opacity < 1, dots are visible during the drawing process. This was not the case prior to this PR being committed.
2. Sometimes, while in the middle of drawing, the brush will simply stop being drawn and will take several seconds to start getting drawn again.

Here is a video of using fabricjs on my project with the latest commit of fabricjs which includes the drawDots PR:
https://youtu.be/wYfd5IohsWY
You can see the performance/lag issue in the video.

The exact "draw dots included" build that I'm using, for reference, is:
https://github.com/AndrewJDR/fabric.js/commits/build_of_a267b76e8
(installed with npm install 'https://github.com/AndrewJDR/fabric.js#build_of_a267b76e8'

---

Here is a video of using fabricjs on my project with the latest commit of fabricjs but with the drawDots commit reverted:
https://youtu.be/QQUvphB4sNc
You can observe that there are no dots here, and the performance is fine.

The exact build "draw dots reverted" build I'm using, for reference, is:
https://github.com/AndrewJDR/fabric.js/commits/pencilbrush_drawdot_reverted
(installed locally with npm install 'https://github.com/AndrewJDR/fabric.js#pencilbrush_drawdot_reverted'

I've included a jsfiddle here where you can easily see the dots:
http://jsfiddle.net/uk7jvr6o/12/
Unfortunately, as for the performance issue (number 2 in the list above), I have not been able to replicate it outside of my web application, which is rather large and there's not a public link of it available at the moment. All I can say is that even with my application, reverting the drawDots PR fixes the performance issue altogether. I know that this is not very helpful at the moment. Perhaps later when I have some more time, I can try to extract enough of my application into a smaller example so I can reproduce the issue. I actually started to do this, but it was getting pretty time consuming. If there's anything less demanding (inserting a few debugging statements, providing some logs, checking something in the webdev console, etc) that you can suggest in the meantime to try to track it down, I'd be happy to help with that.

For now, on my end I'll be running a fork of fabric.js with this drawDots commit reverted.

[AndrewJDR]

I just found this:
#4857

Which already covers the "dots" problem, but doesn't seem to mention the performance issue.

[asturur]

Can we reproduce the perf problem ?

[AndrewJDR]

I cover it in that last paragraph above ^ Pasting below:

Unfortunately, as for the performance issue (number 2 in the list above), I have not been able to replicate it outside of my web application, which is rather large and there's not a public link of it available at the moment. All I can say is that even with my application, reverting the drawDots PR fixes the performance issue altogether. I know that this is not very helpful at the moment. Perhaps later when I have some more time, I can try to extract enough of my application into a smaller example so you can reproduce it on your end. I actually started to do this, but it was getting pretty time consuming. If there's anything less demanding (inserting a few debugging statements, providing some logs, checking something in the webdev console, etc) that you can suggest in the meantime to try to track it down, I'd be happy to help with that. For now, on my end I'll be running a fork of fabric.js with this drawDots commit reverted.

[AndrewJDR]

I will try to do some more debugging on my end and post my results here

[asturur]

I have no idea why redwraing just a part shoul be slower than redrawing everyhing everytime, also in your video it happens pretty fast.

More than a video, you can just open the chrome dev tools, record the performances, start to draw and wait for the per problem to happen. stop to recording, save the profile and share the profile so i can inspect it. The freeze answer is there.

[AndrewJDR]

I'm not sure what you mean by 'also in your video it happens pretty fast', but if you mean that it is performing normally, maybe you looked at the other video link. Here's the one where it is slow:
https://youtu.be/wYfd5IohsWY
(see between 0:04 and 0:06 where it stops drawing altogether -- it's obviously not normal)

It's possible that it's not a performance issue at all, but just a code issue with how the line is drawn during mousemove. I'll do some more investigation now, and save a profile...

[AndrewJDR]

After more looking into it, I've sadly discovered that this performance issue is a bug in chrome. Firefox doesn't have the problem, and furthermore the problem goes away in chrome if I disable "Accelerated 2D canvas" in chrome://flags. Furthermore, I'm on linux and I wouldn't be surprised if this is a linux-only issue.

I will have to attempt building a minimum testcase and report it at bugs.chromium.org.

Thanks.

[AndrewJDR]

Just to document what I did try:
I refactored things so that requestAnimationFrame is used to draw the paths instead of drawing them inside 'mousemove'. This didn't help. Again, this is a bug in chrome.

As a side note, would it be better to do all canvas drawing inside of a requestAnimationFrame loop? Something like:

        _lineRender: function() {
            var points = this._points;
            var length = points.length;
            var ctx = this.canvas.contextTop;
            var curPoint = this._curPoint;

            if (length > 1)
            {
                for (let ii = curPoint; ii < length-1; ii++)
                {
                    ctx.beginPath();
                    ctx.moveTo(points[ii].x, points[ii].y);
                    ctx.lineTo(points[ii+1].x, points[ii+1].y);
                    ctx.stroke();
                }
                this._curPoint = length - 1;
            }

            window.requestAnimationFrame(this._lineRender.bind(this));
        },
        onMouseMove: function(pointer) {
            this._captureDrawingPath(pointer);
        },

... This is an incomplete implementation just to give an idea. I'm wondering if it's better for performance, rather than having drawing code happening inside of mousemove which can perhaps happen more than once per frame.

[asturur]

having the recorded profile would let you know what is taking too much.
FabricJS generally render in the requestAnimationFrame, but the pencil brush just react to mouse move, it could render all the captured items since last frame, maybe is a possible improvements for all brushes.

[AndrewJDR]

Here is jsfiddle that reproduces the problem for me:
http://jsfiddle.net/uk7jvr6o/12/

Here is a video showing and explaining the problem (2 minutes):
https://youtu.be/RK8QKim4uWs

The problem only happens with the css object-fit:contain setting on the canvas. There is nothing in the performance output that is indicative of a utilization issue. Again, this appears to be a chromium/chrome bug, so I will pass this along to the chromium team.

The result on windows is even worse than it is on linux, looks like this (animated gif):

[AndrewJDR]

Chromium bug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=840075

[AndrewJDR]

Also, just for the sake of comparison here is the same jsfiddle that uses fabric.js v2.0.2:
http://jsfiddle.net/xhy9sqxj/3/

This one works smoothly, because fabric.js v2.0.2 re-renders the entire path on every mousemove which does not trigger the chromium bug.

[asturur]

I have no idea what trigger the chromium bug, drawing a path or 400 paths, is the same code, the only difference is a clearContext happening.

this is a picture of the profile, as you can see there is a frame lasting more than half second that is the moment in which it looks frozen.
Except that is not fronze at all, mouse move is firing, processed and the fabric code is running smoothly, just the browser is not rendering at all.

each mousemove under a long lag of 2 seconds is being executed in 0.24ms that looks good to me.

Is not a performance problem, is just a browser bug

[asturur]

If you want a chance to have the bug looked at, you should reproduce it in plain JS, a bug based on a library is not gonna considered at all.

[AndrewJDR]

@asturur Yes, good idea. I made a pure js one here http://jsfiddle.net/n7nebbL8/16/
Thanks

[AndrewJDR]

Another useful bit of info, in case anyone else runs into this same browser bug and finds this github issue later:
This browser bug is also present in Safari on iOS, and probably Safari on MacOS as well.

This means that the bug probably pre-dates the webkit/blink fork in 2013, so it's a long-standing issue. At some point I will file a webkit bug as well.

Again, I'll point out: this bug only happens if you're using object-fit:contains on your canvas element(s), so if it's possible for you to simply NOT use object-fit:contains, that is a valid workaround.

Unfortunately, in our case object-fit:contains is necessary, so I will have to revert fabric.js commit f855bbc in my own private fork of fabric.js as a workaround for the browser bug. If you're in the same situation, you can do the same thing. (Until chromium and webkit fix this browser bug, which could be a while or may never happen...).

[asturur]

yes i work on macOS and i could see it.

Something you should put the detail on:
http://jsfiddle.net/n7nebbL8/17/

the more the canvas is resize by css, the more the bug is visible, so if the tester has a big screen ( like a 4k ) and the fiddle does not resize the canvas under 1000px, the bug may not happen. If you use this fiddle in which the container has 100px by 200px container, the bug is immediate and more visible

[asturur]

Is still unclear to me why our code trigger the bug, but i feel like you may have manymore places in fabricJS that will trigger it. You are triggering with line to, pencilbrush is using quadraticTo, it may happen with more stuff.

[AndrewJDR]

@asturur That's a good note, I will make mention of it on the chromium/webkit bugs and update with the new link

Another thing I'll mention: If there is demand for it, I can submit a PR that adds a flag to the fabric.Canvas that allows for toggling of the "clearContext" behavior. It's totally up to you if you want the extra code complexity added for that, just let me know.

[asturur]

i do not want to work around bugs with patches, they fall forgotten and is not the way things should go. You can patch your application if it has the need to be live with this bug in the wild.

Is also unclear why you need the object-fit, you can also work around that resizing the canvas with the container, if you need then a bigger image, you can still make it bigger when you export it.

You can work on a 1000px space coordinates on a smaller area using the zoom, and inspecting the container for size and actually resize your canvas instead of css doing it.

Performance should benefit too.

[AndrewJDR]

Is still unclear to me why our code trigger the bug, but i feel like you may have manymore places in fabricJS that will trigger it. You are triggering with line to, pencilbrush is using quadraticTo, it may happen with more stuff.

@asturur Yes, I'm worried about this. What this means is that every time I run into the browser bug, I will have to implement some kind of "clearContext-like" patch as a workaround until the browser vendors fix the bug. And that might not even be possible in certain situations! I just don't know the fabric code well enough offhand yet to know for sure if such workarounds will be possible for the other areas of fabric.

It's also possible that there's a better workaround for the problem rather than calling clearContext(), but I would have to spend some more time trying to discover that.

Is also unclear why you need the object-fit, you can also work around that resizing the canvas with the container, if you need then a bigger image, you can still make it bigger when you export it.

In our application, the fabric canvas is overlaid on top of a video element which needs to maintain the aspect ratio when the browser window gets scaled (using object-contain:fit). Fabric needs to match that. I'm not sure how to do maintain aspect with video scaling -- css percentages are not really accurate enough. It may be possible though, I will have to spend a bunch of time tinkering around with css to find out.

And now that you mention it, I'm thinking about this more. I may be able to use object-fit:contain only during rapid "onresize" calls (in order to maintain fast browser window resizes), then remove the object-fit:contain just after all the resizing is totally complete. I already have similar handling in place that changes the backing store dimensions so the canvas is at 1:1 pixel ratio once a window resize is totally complete (so that I avoid having the canvas scaled down).

[AndrewJDR]

You can work on a 1000px space coordinates on a smaller area using the zoom, and inspecting the container for size and actually resize your canvas instead of css doing it.

Right, I actually already do something like this. the object-fit:contain is to maintain the performance and accuracy (i.e. aspect ratio of the canvas matches the video that it's overlaying) of the canvas when the user does interactive browser window resizes -- I've found that doing canvas backing store resizes in a javascript onresize handler is very slow and even crash-prone. Once the resize operation is totally done, I change the dimensions of the canvas store and reposition/scale the elements appropriately -- I essentially implemented my own setZoom because it gave me a bit more control over some things.

[asturur]

Yes i normally debounce the resize event, scaling with css while resizing and do the real resize when they mouse up.

[AndrewJDR]

... thankfully I was able to take my existing resize debounce logic and include adding/removing `object-fit:contains' from the canvas as appropriate. So object-fit:contains is now only used during the resize interaction which is fine since the browser bug isn't triggered at that time. Whew! Thanks for the help.