Hi Dani,
Welcome, and thank you for taking a look at this problem.
I don't know enough yet to suggest the best course of action, but I took
a quick look at it. I only see the one call to the native
RenderingQueue::flush -- from freeSpace(int) if autoFlush is true.
RenderingQueue::flush does a Java upcall to WCRenderQueue::fwkFlush
which then calls WCRenderQueue::flush to decode and process the buffer.
The native RenderingQueue::flushBuffer method does a Java upcall to
WCRenderQueue::fwkAddBuffer which will add the buffer to a linked list
of buffers, and then allocate a new BufferData wrapper object. It then
calls WCRenderQueue::flush if the total size of the linked list of
buffers it too large.
One thing that should be checked is whether the call to
WCRenderQueue::flush from native RenderingQueue::flushBuffers and then
again from RenderingQueue::flush is causing the problem. Instrumenting
the code on the Java side (which is usually easier if it gives you the
information you need), might show what's going on.
-- Kevin
On 10/20/2024 7:01 AM, dani-kurmann wrote:
Hi everyone, nice to be here.
I don't want to be overzealous, but I did sign up for a bugfix :)
So here is me, socializing:
I ran into the bug in a similiar scenario as described in the Bug
report: https://bugs.openjdk.org/browse/JDK-8229902
It's a buffer-problem. The culprit is the flush()-call on line 64 of
/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/RenderingQueue.cpp
(in the freeSpace(int size) function)
if (m_buffer && !m_buffer->hasFreeSpace(size)) {
flushBuffer();
if (m_autoFlush) {
flush();
}
}
this gets called, before a shape(or possibly something else) is added
to the bytebuffer. If there is not enough space left in the buffer,
here we go:
The flushBuffer()-Call works fine. It sets up new Buffer Space as
intended and, I guess, flushes the buffer.
The flush()-call however leads to the described Bug. Just comment it
out to test it. But thats not a bugfix, of course.
Its very interesting where the two calls lead, this is right in the
sweet spot between java and c++ and openjdk and webkit. I dont claim
to understand half of it. But I do find it very interesting.
This is the point where I ask for help: I can propose a bugfix, but I
want to understand the issue deeper. Maybe this is a newbie-problem,
but I have problems understanding the above mentioned sweet spot. So
any advice is very welcome!
Please DO correct me, and show me that other approach that I dont see,
but the way I see it, there's two ways to approach this thing:
A) Treat it as an initialisation-problem. e.g. There is nothing wrong
with RenderingQueue, it just needs to be initialised with
m_autoFlush=false. (in this case, at least)
B) There seems to be something strange going on in RenderingQueue,
understand it and fix it.
A) is a lot easier: just follow back to where the RenderingQueue
Constructor gets called:
/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/ImageBufferJavaBackend.cpp,
line 82:
auto context = makeUnique<GraphicsContextJava>(new
PlatformContextJava(wcRenderQueue, true));
The second Argument for the PlatformContext-constructor sets the
m_autoFlush-Variable upon which the flush()-call in the
freeSpace-function in RenderingQueue.cpp is conditional. Set it to false.
as a git commit:
https://github.com/CodeMonkeyIsland/jfx/commit/96f23307c7b6f324bac416b90c0eac4ad40b13a8
pro:
-simple fix
-no need to mess around in more than one file
-looks like this is a normal way to use RenderingQueue
con:
-more and more feels like a hack
B) is can of worms: You can comment out not only the flush()-call in
the freespace-function, but the flush() function itself. At first, I
thought, that the flush()-function is still needed for a flush-call
during flushBuffer() outside RenderingQueue. But that was not the same
flush()-function. I just tested that, and it works. (havent checked
yet, if this breaks something else)
So assuming, the flush()-function is really defunct and not needed(and
I would have to dive into this a lot deeper to make that statement as
a fact), this is a chance to do some spring-cleaning in
RenderingQueue. Again, I only see two main ways of approaching this:
- make m_autoFlush obsolete. Since (if the assumption above is right)
it already flushes regardless of how m_autoFlush is set, lets stop
pretending. Remove m_autoFlush, adjust the constructors and the
constructor calls outside RenderingQueue.
- make m_autoFlush great again :) It would basically mean separating
flushBuffer() into two functions, one that does the addBuffer-part and
one that does the flushing. Then do the addBuffer unconditional and
the flushing conditional on m_autoFlush().
The "problem" here being, that other parts of the codebase seem to
have gone the way of approach A). So another piece of code
initializing RenderingQueue with m_autoFlush=false doesnt mean that
they dont want to autoFlush. So every call of the constructor has to
be analysed, if the intention is to autoflush or not. I dont see the
use-case for not flushing. So this approach might very well lead to a
situation, where every single constructor call sets m_autoFlush to
true and m_autoFlush is kind of obsolete again. Or not. Anyone has an
example, where it shouldnt flush? Maybe there is runtime to be
optimised by planning when to flush? I dont know...
pro:
-if there is a problem in RenderingQueue, then thats the place to fix
it! No matter the consequences.
con:
-even if its an easy fix in RenderingQueue, it will change its
behaviour. Or at the very least make autoFlush obsolete. This has
consequences.
Approach B) seems a lot scarier than A), but if it must be done, it
should be done. If you guys think this is the rabbit hole to be, I
would be willing to go there. However, I don't feel competent to
overview all the consequences of this. I could really need some advice
and help on that road, I guess thats why Im here.
I could imagine doing A) as a temporary fix and an exercise. Then
exploring B) deeper with some help.
As for testing: At first glance, this seems like a non-automated Mk.1
eyeball-test. On a closer look however, you could do something like
the Bug-Class in the bug-report, extract the
RenderingContext-imagedata, pass it from js to java and check the
color of the pixel, to see if a shape has been drawn. A window would
have to be opened for this test, I dont know if thats allowed. (But
why not, I guess)
So what do you guys think? Does that sound like a plan? Does this
break something, that I don't see?
I am new here. Anyone willing to have a look into this and maybe point
me in the right direction sometimes?
kind regards
Dani Kurmann
p.s. the Bug class in the bug report does not work anymore.(at least
for me, forked or unforked) the script in executeScript doesn't get
executed. As a quick fix, you can pass the function in the html
document like so:
WebEngine engine = webView.getEngine();
String content="<html><body><canvas id=\"canvas\" width=\"600\"
height=\"600\" style=\"border: 1px solid
red;\"></canvas><script>window.onload = function() {";
content=content
+ " var canvas = document.getElementById('canvas');"
+ " var c = canvas.getContext('2d');"
+ " var size = 4;"
+ " var step = size + 2;"
+ " for (var y = 0; y <= canvas.height; y = y + step) {"
+ " for (var x = 0; x <= canvas.width; x = x + step) {"
+ " c.fillRect(x, y, size, size);"
+ " }"
+ " }";
content=content+"}</script></body></html>";
engine.loadContent(content);
Sent with Proton Mail <https://proton.me/mail/home> secure email.
