[OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception
Ajit Ghaisas
ajit.ghaisas at oracle.com
Wed Mar 30 12:12:19 UTC 2016
Hi,
Thanks Sergey and Phil for answering some of Jim's concerns regarding this fix.
I have answered rest of them below:
Raster.java, line 742: This branch of the switch statement instantiates a SWR which doesn't have a specific buffer type. It could be replaced with just "break;" and let the code after the switch create the raster (it can't be deleted from the switch statement because the default would throw an exception, but it could just "break;" on that dataType). At a minimum, though, it didn't need the instanceof or cast that was added since it will just fall through to exactly the same code anyway.
>>> [Ajit] : Good catch. As suggested, I have replaced this with just a 'break' statement. It's much cleaner now. Thanks.
Raster.java, line 894: Why was the test for dataType removed from this if statement?
>>> [Ajit] : It was an extra check. It was removed after Phil's review comment.
The check for 'dataType' is removed as it is populated from 'dataBuffer' (and not from SampleModel as in other methods).
The newly added check (dataBuffer instanceof DataBufferByte) that uses 'dataBuffer' is sufficient to decide whether to create BytePackedRaster or SunWritableRaster.
Raster.java: line 980: Something to file as a potential bug for future work. The test for getSampleSize(0) in this method is much more permissive than the test for bitsPerPixel in createPackedRaster just above this method. Both determine whether to return a BytePackedRaster so they should probably agree.
>>> [Ajit] : I agree. The code here can be made in line with test for bitsPerPixel as in createPackedRaster method. I will create a bug for this.
Raster.java, line 986: Something to file as a potential bug for future work since the fix would have to be verified that it doesn't disrupt the other parts of this method, but... The set of if statements in this method never checked for a BandedSampleModel to try to create a BandedRaster as in createBandedRaster. On the other hand, no developer has complained so far (that I know of, but maybe we have an old bug filed on it that I'm not aware of).
>>>[Ajit] : Without my fix, it was easier to get RasterFormatException as explained in Bug Description. This fix enhances the code by making type specific Raster class constructors type-safe.
Now, the bigger question is: Whether methods in Raster.java support all possible combinations of Raster creations?
What I can say is - this fix does not break anything than what is already present in Raster.java.
If there is specific user complaint or any existing bug - we can look into it separately.
Raster.java: the same comments for createRaster above apply for createWritableRaster at line 1033.
The following 2 comments are identical for all of the raster types {
ByteBandedRaster.java, line 76: This code will throw a ClassCastException
now instead of the RasterFormatException that used to be thrown.
Do we care? It would be easy enough to add a test to just this
method (and the similar versions in other files).
>>> [Ajit] : Since we have decided to make the type specific Raster class constructors type safe, yes, it is possible to get ClassCastException. But, as the usage of these classes is guarded now with "instanceof" checks, it would be rare.
ByteBandedRaster.java, line 668: Extra credit: This cast could be avoided
if we make SunWritableRaster employ generics for a strongly typed DataBuffer.
>>> [Ajit] : the casted member ' dataBuffer' on this line is a member of Raster class.
The class hierarchy of ByteBandedRaster is ( added on single line for convenience)
-------- ByteBandedRaster extends SunWritableRaster extends WritableRaster extends Raster.
I am not quite clear on how we can avoid cast on this line. Can you please elaborate?
}
ShortInterleavedRaster - I think the import of DataBuffer can be eliminated now?
>>> [Ajit] Good catch - I have eliminated the extra import now. Thanks.
Here is the updated webrev:
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.05/
Request you to review it.
Thanks,
Ajit
On 3/28/16 3:18 AM, Ajit Ghaisas wrote:
> Hi,
>
> Thanks Jim for thorough review.
>
> Here is the updated webrev :
> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.04/
>
> In this update :
> 1) I have corrected missing space after casting
> 2) I have modified code to adapt suggested indentation for blocks having conditionals & method declaration split on multiple lines.
>
> Please note that, I have done indentation change only if it is related to the code changes done as part of this fix.
> The files in this review already have indentation issues and fixing all of them will result in multiple changes masking the actual code changes that fixes the reported issue.
>
> Request you to review the updated webrev.
>
> Regards,
> Ajit
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, March 24, 2016 5:59 AM
> To: Phil Race
> Cc: 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation
> of a WritableRaster with a custom DataBuffer causes erroneous
> Exception
>
> I should point out that this form I demonstrate below is only used when there are continuation lines on the prefix to the code block (conditionals, method declarations, loop controls, etc.), so:
>
> if (some simple condition) {
> ...
> }
>
> and:
>
> if (some complex condition &&
> some additional condition)
> {
> ...
> }
>
> but, not:
>
> if (some single line condition)
> {
> // blech
> ...
> }
>
> The reason for this is that the standard indentation would recommend:
>
> if (some complex condition &&
> some additional condition) {
> // code block
> }
> or
> void foomethod(int sx, int sy,
> int dx, int dy) {
> // code block
> }
>
> which may be more compact, but the lack of a breaking line means you have to vary the indentation of the declarations/conditionals and as a result they don't line up nicely which makes them harder to scan if they are all related (and frequently in graphics code you end up with a series of very similar arguments or conditionals that are more readable if they line up nicely), and the only indication of when the multiple lines of continued declaration/conditionals end and the body of the method begins is the number of spaces - noting that frequently the indentation on lines in practice is just wrong so this form makes it hard to distinguish between "that line is a continuation line" and "someone indented that line wrong"...
>
> ...jim
>
> On 3/23/16 5:14 PM, Jim Graham wrote:
>> For the record, in many places in the 2D code we also adopt a slight
>> extension of the indentation rules such that the below might be written as:
>>
>> public ByteBandedRaster(SampleModel sampleModel,
>> DataBufferByte dataBuffer,
>> Point origin)
>> {
>> // body lines of the method...
>> }
>>
>> with the open curly brace on the following line so that it more
>> visually points out the beginning of the body block of the method and
>> it's easy to find the start/end of the block by sighting down the
>> left margin. The official policy is for the "{" to be at the end of
>> the previous line after "Point origin) {" and as more new engineers
>> work on the code and follow the official rules, the above form
>> becomes less common. (Sad face since I particularly like the above
>> form...)
>>
>> ...jim
>>
>> On 3/22/16 4:10 PM, Phil Race wrote:
>>> Ajit,
>>>
>>> There is also some odd indentation in ByteBandedRaster.java which is
>>> not yours but
>>>
>>> 98 public ByteBandedRaster(SampleModel sampleModel,
>>> 99 DataBufferByte dataBuffer,
>>> 100 Point origin) {
>>>
>>>
>>> This appears to be the result of someone using tabs a long time ago.
>>>
>>> Since you are touching the method signature lines anyway may be
>>> better fixed so we have these aligned
>>>
>>> public ByteBandedRaster(SampleModel sampleModel,
>>> DataBufferByte dataBuffer,
>>> Point origin) {
>>>
>>> [not sure if that will make it through mail as intended].
>>>
>>> Here in Raster.java, the first condition now seems to be redundant ..
>>> 890 if (dataType == DataBuffer.TYPE_BYTE &&
>>> 891 dataBuffer instanceof DataBufferByte &&
>>>
>>>
>>>
>>> -phil.
>>>
>>>
>>> On 03/22/2016 03:28 PM, Jim Graham wrote:
>>>> Hi Ajit,
>>>>
>>>> Most of your if statements are spaced wrong. There should be a
>>>> space between "if" and the parentheses. I'll review more later,
>>>> but I noticed that issue in the first couple of files I looked at...
>>>>
>>>> ...jim
>>>>
>>>> On 3/15/16 7:05 AM, Ajit Ghaisas wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks Sergey and Jim for suggestions.
>>>>>
>>>>> I have made the data specific Raster constructors type safe
>>>>> now. Also, I have modified all Raster creations in Raster.java to
>>>>> support custom DataBuffer classes.
>>>>>
>>>>> Please review the changes present in updated webrev :
>>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.02/
>>>>>
>>>>> Regards,
>>>>> Ajit
>>>>>
>>>>> -----Original Message-----
>>>>> From: Jim Graham
>>>>> Sent: Friday, March 11, 2016 2:40 AM
>>>>> To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev
>>>>> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518:
>>>>> Creation of a WritableRaster with a custom DataBuffer causes
>>>>> erroneous Exception
>>>>>
>>>>> Yes, those constructors should be type-safe. Perhaps that was
>>>>> done to save code in the caller, but that is a small price to pay
>>>>> to avoid errors in the future, and it makes sure there is a backup
>>>>> plan for different kinds of buffers...
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 3/10/16 4:48 AM, Sergey Bylokhov wrote:
>>>>>> Hi, Ajit.
>>>>>> What about other cases in Raster.java, where the DataBuffer is
>>>>>> passed as a parameter? Probably we can simplify the code and find
>>>>>> all such issues if we changes the ByteInterleavedRaster/etc. For example:
>>>>>>
>>>>>> ByteInterleavedRaster(SampleModel sampleModel,DataBuffer
>>>>>> dataBuffer,...) to
>>>>>>
>>>>>> ByteInterleavedRaster(SampleModel sampleModel,DataBufferByte
>>>>>> dataBuffer,...)
>>>>>>
>>>>>> Because we throw an exception in such cases anyway:
>>>>>>
>>>>>> if (!(dataBuffer instanceof DataBufferByte)) {
>>>>>> throw new RasterFormatException("ByteInterleavedRasters
>>>>>> must have "
>>>>>> + "byte DataBuffers");
>>>>>> }
>>>>>>
>>>>>> And the compiler will help us, what everybody think about it?
>>>>>>
>>>>>> On 09.03.16 17:38, Ajit Ghaisas wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Modified the test and added check for
>>>>>>> MultiPixelPackedSampleModel condition.
>>>>>>>
>>>>>>> Please review updated webrev :
>>>>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.01/
>>>>>>>
>>>>>>> Regards,
>>>>>>> Ajit
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Sergey Bylokhov
>>>>>>> Sent: Wednesday, March 09, 2016 4:06 PM
>>>>>>> To: Ajit Ghaisas; awt-dev at openjdk.java.net; Semyon Sadetsky;
>>>>>>> Ambarish Rapte; 2d-dev
>>>>>>> Subject: Re: [9] Review-request for 6353518: Creation of a
>>>>>>> WritableRaster with a custom DataBuffer causes erroneous
>>>>>>> Exception
>>>>>>>
>>>>>>> Changes for 2d area.(cc)
>>>>>>>
>>>>>>>
>>>>>>> On 07.03.16 11:20, Ajit Ghaisas wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-6353518
>>>>>>>>
>>>>>>>> Issue : (Text from bug description)
>>>>>>>>
>>>>>>>> An attempt to create a WritableRaster via
>>>>>>>> Raster.createWritableRaster(SampleModel sm, DataBuffer db,
>>>>>>>> Point
>>>>>>>> location) using a custom DataBuffer causes an erroneous
>>>>>>>> RasterFormatException.
>>>>>>>> Apparently the reason for this bug is that
>>>>>>>> IntegerComponentRaster insists on being passed an instance of
>>>>>>>> DataBufferInt rather than just a DataBuffer with a DataType of TYPE_INT.
>>>>>>>> This is quite annoying since DataBufferInt is declared final
>>>>>>>> and thus cannot be extended.
>>>>>>>>
>>>>>>>> Fix :
>>>>>>>>
>>>>>>>> The last line of Raster.createWritableRaster() method already
>>>>>>>> has a way to handle generic case if the input does not match
>>>>>>>> any of the cases in switch statements in that method.
>>>>>>>>
>>>>>>>> The fact that " *InterleavedRaster() constructors throw
>>>>>>>> exception if DataBuffer passed to them does not match the
>>>>>>>> expected type" was ignored earlier.
>>>>>>>>
>>>>>>>> This fix adds a check of "DataBuffer type" before creating
>>>>>>>> respective
>>>>>>>> *InterleavedRaster() objects.
>>>>>>>>
>>>>>>>> It constructs the SunWritableRaster in case DataBuffer type
>>>>>>>> does not match any data specific DataBuffer classes
>>>>>>>> (DataBufferByte, DataBufferUShort, DataBufferInt)
>>>>>>>>
>>>>>>>> Request to review webrev containing this fix :
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.00/
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Ajit
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>>>>
>>>>>>
>>>>>>
>>>
More information about the 2d-dev
mailing list