<AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Jim Graham]

Hi Anton,

On 12/11/13 5:13 AM, Anton V. Tarasov wrote:
> Hi Jim,
>
> On 11.12.2013 13:14, Jim Graham wrote:
>>
>> In BufferedImage, the private fields and methods will require an
>> accessor method for the inner class to access them.  Is there a reason
>> they can't be left package-accessible?
>
> Sorry, I don't clearly understand what you mean... The "scale" private
> field and the new methods are accessed from other packages, so I can't
> make them package-private. I've added the accessor which I put into
> OffscreenImage (an internal class).

This is entirely an internal detail related to how inner classes are 
implemented (like the class that you use to allow OffscreenImage to 
access the BI methods).

The inner "accessor helper" class is technically outside the class scope 
of the BufferedImage class - it is a separate class and it is restricted 
in the byte code interpreter to the same access privileges as any other 
"separate class", but the methods it calls on BI are private.  The 
runtime does not allow this, but the compiler wants you to think this is 
possible for convenience of writing inner classes.  As a result, it 
injects fake accessor calls into the BI class that allow the inner 
classes to call private methods on BI.  Dump the byte codes of the 
relevant classes with javap and you will see the shadow calls that break 
the privacy of those methods.  Those methods are not allowed from java 
code, only the inner class magic can use them.  It is byte-code magic to 
enable the syntactic sugar of inner classes.

I believe if you just make the new methods on BI package-private instead 
of fully class-private, then those helper inner-class-only accessor 
methods aren't needed.

>> In JViewport, is there a reason why a map of various scaled backing
>> stores are kept rather than just validating the existing backing store
>> against the new desired scale and replacing it when it changes?  Does
>> the scale on the backing store ping-pong back and forth between scales
>> much?
>
> This doesn't ping-pong much, indeed. So you want to say using a cache
> here is not justified? Ok, why I did that is probably to mimic the logic
> used by RepaintManager which does use a map b/w graphic configs and
> volatile images (the volatileMap field), whereas changes of the
> GraphicsConfig shouldn't occur often.
>
> Anyway, this indeed doesn't increase performance but increases a
> footprint, so I'm ok not to use cache here.

I just wanted an analysis of whether the new complexity had any value. 
It sounds like it mirrors what was done in other places and that is 
fine, but perhaps this particular variant of that similar mechanism is 
premature?  In general, I'd wait to add complexity until a problem is 
demonstrated that makes it worthwhile.

>> BufImgSurfaceData - you have to validate the transform as not flipping
>> or rotating (even quadrant rotation will violate your conditions.  I
>> believe that testing the transform type with a mask consisting of the
>> TRANSLATE and SCALE_MASK flags should be fine. Also, do we wan to set
>> a precedent of allowing copyArea under an arbitrary scale, or should
>> we test it to make sure it is specifically the same scale as the
>> device scale factor?
>
> Ok, I'll check the transform for TRANSLATE/SCALE only. The same scale as
> the device works for me, so I'll limit it to that case.

Though, I do think we should eventually have copyarea work with scales.

BTW, this same code appears in SG2D, but it is rejected by a test on the 
transform type.  Have you thought about simply inserting your transform 
code into the test for the transform type in SG2D rather than shadowing 
the entire method?

>> SG2D - I'm worried about the performance impliciations of adding a new
>> method in that creates and returns an array on every drawImage call.
>> Did you do a performance test with J2DBench to verify the impact of
>> these operations?
>
> Well, this was done just to make the code more clear. I can remove that
> method (and do the instanceof check in every place I called it), or as
> an alternative I can cache the array so that not to create it (out of
> the stack) every time. Does this make sense to you? (I didn't run
> J2DBench yet).

A single SG2D should be single threaded (calling from multiple threads 
is not supported and results are not guaranteed) so caching the array in 
the instance would be fine.  Also, we are discussing the issue of 
getWidth(null) not matching the logical size of the image in another 
thread.  Depending on what we decide there, it may make sense to move 
the scaling logic inside the getWidth() methods of the scaled buffered 
images and then we need far fewer mods to SG2D.  But, I think that would 
only work reasonably if such "scaled buffered images" are never leaked 
to the developer until we come up with a reasonable external API that 
makes sense for them.

I'm going to save the rest of the reply for another message since it 
gets involved...

			...jim