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

[Anton V. Tarasov]

[cc'ing to j2d]

Jim, let me continue this thread later when a general agreement about a scaled BufferedImage is made.

Thanks,
Anton.

On 12.12.2013 1:44, Jim Graham wrote:
> 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