On Sun, 26 Jan 2020 11:58:27 GMT, Frederic Thevenet 
<github.com+7450507+ftheve...@openjdk.org> wrote:

>>> 
>>> 
>>> > the `WriteableImage` used to collate the tiles and the tiles returned 
>>> > from the `RTTexture` have different pixel formats (`IntARGB` for the tile 
>>> > and `byteBGRA` for the `WriteableImage`).
>>> 
>>> Where did you see these?
>> 
>> Simply by watching the value of  `tile.getPixelReader().getPixelFormat()` 
>> and `wimg.getPixelWriter().getPixelFormat()` before doing the copy at 
>> https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>>> 
>>> > Unfortunately it seems the only way to choose the pixel format for a 
>>> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one 
>>> > can no longer use a `PixelWriter` to update it...
>>> 
>>> You can update it with `PixelBuffer#updateBuffer`. I think that you will 
>>> want to pass the stitched tile as the dirty region.
>> 
>> Thanks. I'll look into it.
> 
>> 
>> 
>> > profiling a run of the benchmark shows that a lot of time is spent into 
>> > `IntTo4ByteSameConverter::doConvert`
>> 
>> This is a bit naive, but what if you parallelize the code there? I didn't 
>> test that this produces the correct result, but you can try to replace the 
>> loops with this:
>> 
>> ```
>> IntStream.range(0, h).parallel().forEach(y -> {    
>>     IntStream.range(0, w).parallel().forEach(x -> {
>>         int pixel = srcarr[srcoff++];              
>>         dstarr[dstoff++] = (byte) (pixel      );   
>>         dstarr[dstoff++] = (byte) (pixel >>  8);   
>>         dstarr[dstoff++] = (byte) (pixel >> 16);   
>>         dstarr[dstoff++] = (byte) (pixel >> 24);   
>>     });                                            
>>     srcoff += srcscanints;                         
>>     dstoff += dstscanbytes;                        
>> });                                                
>> ```
> 
> I don't think this works as it is, as all threads race to increment `srcoff` 
> and `dstoff`.

I would be very cautious of using multi-threading here. In any case, I think 
that the issues around absolute performance could be handled separately.

Having said that, given my review comments, along with the concerns over 
performance regressions for those cases that will now be tiled, but formerly 
weren't, I no longer think this is a good candidate for openjfx14. This PR 
should be retargeted to the `master` branch for openjfx15.

-------------

PR: https://git.openjdk.java.net/jfx/pull/68

Reply via email to