[OpenJDK 2D-Dev] [9] RFR JDK-8164931 : Verify if writer.abort() works properly for all writers in IIOWriteProgressListener.

[Philip Race]

Seems OK .. modulo Brian's off-list question about TIFF

-phil.

On 9/20/16, 10:57 PM, Jayathirth D V wrote:
> Hi Prasanta,
>
> Thanks for your review.
> When we are writing embedded image we have internal WriteProgressAdapter in which we have dummy listener functions which are doing nothing.
> So there is no need to check for abortRequested() at 1340  processImageProgress(percentageDone); in BMP.
>
> Regarding GIF change I want to keep abortRequested()checks right after writeRows() or writeRowsOpt() for readability and debugging purpose. Only in last else case we can merge the request which will look different from all other if conditions.
>
> I have modified test case to remove not needed catch clause. Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8164931/webrev.02/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prasanta Sadhukhan
> Sent: Wednesday, September 21, 2016 11:00 AM
> To: Jayathirth D V; Sergey Bylokhov; Philip Race; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8164931 : Verify if writer.abort() works properly for all writers in IIOWriteProgressListener.
>
> Is it not required to call abortRequested() after
>
> 1340                         processImageProgress(percentageDone);
>
> in BMP
>
> In GIF 1044 if (abortRequested()) {
> 1045 return;
> 1046 } and 1054                 if (abortRequested()) {
> 1055                     return;
> 1056                 }
>
> can be merged and called after if-else block!!
>
> In the testcase, I believe we do not need to have try-catch-finally block, just a try-finally block should do as we are not doing anyprocessing in catch block.
>
> Regards
> Prasanta
> On 9/16/2016 5:26 PM, Jayathirth D V wrote:
>> Hi Sergey,
>>
>> Thanks for your review.
>> abortRequested in insert() should be at the end of the function only.
>> It has come by mistake while I was creating webrev as I was testing other things.
>> Please find the updated webrev with abortRequested() check at the end of insert() function for review:
>> http://cr.openjdk.java.net/~jdv/8164931/webrev.01/
>>
>> Regards,
>> Jay
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Friday, September 16, 2016 4:36 PM
>> To: Jayathirth D V; Philip Race; Prasanta Sadhukhan; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8164931 : Verify if writer.abort() works properly for all writers in IIOWriteProgressListener.
>>
>> Looks fine, expect the changes in TIFFImageWriter.java. I am not sure
>> I understand why abortRequested was moved from the end of insert();
>>
>> On 16.09.16 12:04, Jayathirth D V wrote:
>>> Hi,
>>>
>>>
>>>
>>> Please review the following fix in JDK9 at your convenience:
>>>
>>>
>>>
>>> This issue is similar to
>>> https://bugs.openjdk.java.net/browse/JDK-4924727 where we made
>>> changes to all ImageReader plugins.
>>>
>>>
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8164931
>>>
>>>
>>>
>>> Webrev : http://cr.openjdk.java.net/~jdv/8164931/webrev.00/
>>>
>>>
>>>
>>> Issue : Verify that when we issue ImageWriter.abort() in
>>> IIOWriteProgressListener callbacks whether writing is aborted properly.
>>>
>>>
>>>
>>> Root cause : In many writer plugins we are not checking for
>>> abortRequested() right after IIOWriteProgressListener callbacks. In
>>> which case writing may continue until we check for abortRequested().
>>> Also in some writers we are not calling clearAbortRequest() before
>>> every
>>> write() call.
>>>
>>>
>>>
>>> Solution : Check for abortRequested() after every
>>> IIOWriteProgressListener callbacks and before every write() call we
>>> should have clearAbortRequest() called.
>>>
>>>
>>>
>>> In case of JPEG clearAbortRequest() is overridden in JPEGImageWriter
>>> and it clears native abort flag also so there is no change in
>>> JPEGImageWriter. WBMP changes will be done in JDK-8164930 as
>>> checkSampleModel() is failing for WBMP.
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>>
>>>
>> --
>> Best regards, Sergey.