[OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort() method does not work when called inside imageStarted for PNG
Philip Race
philip.race at oracle.com
Thu Sep 8 20:46:09 UTC 2016
I wondered what happened to that comment.
I am OK with this so long as Brian is OK with the TIFF changes.
-phil.
On 8/31/16, 11:55 PM, Jayathirth D V wrote:
> Hi Phil& Brian,
>
> Before this code change there was no check for abortRequested() right after processImagestarted() callback in GIFImageReader.java.
> After code change we are checking for abortRequested() right after processImagestarted(). So after this call there is no need to verify for abortRequested() before processImageProgress() so I have moved the logic from while(abortRequested()) { ... } to do {...} while(abortRequested()). I have just removed redundant check for abortRequested().
>
> Also as discussed offline I have made small change in JPEGImageReader.java to remove initialization of "boolean aborted" variable which is not used.
> Also I noticed that previously I was overriding comment present in same file with new comment , I have corrected that mistake.
> Please find updated webrev with the changes in JPEGImageReader.java:
> http://cr.openjdk.java.net/~jdv/4924727/webrev.03/
>
> @Brian : In case of TIFFImageReader, clearAbortRequest() was not called before every read operation, I have added the same. Also I have removed processImageProgress(0.0f) call which I feel is not needed as we have processImageStarted() which will take care of it(Also to maintain callbacks at same intervals as they are in other readers).
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Philip Race
> Sent: Thursday, September 01, 2016 4:48 AM
> To: Sergey Bylokhov
> Cc: Jayathirth D V; 2d-dev; brian Burkhalter
> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort() method does not work when called inside imageStarted for PNG
>
> I don't understand why the change from while() { .. } to do { .. }
> while(..) was
> needed in GIFImageReader but then I don't see any harm in it either.
> Can you explain that one ?
>
> Also I'd like Brian to sign off on the TIFF change.
>
> Other than that all seems fine.
>
> -phil.
>
> On 8/31/16, 5:37 AM, Sergey Bylokhov wrote:
>> On 31.08.16 14:48, Jayathirth D V wrote:
>>> Hi Sergey,
>>>
>>> In case of JPEG whole read process is under a ThreadLock.
>>> public BufferedImage read(int imageIndex, ImageReadParam param)
>>> throws IOException {
>>> setThreadLock();
>>> try {
>>> cbLock.check();
>>> try {
>>> readInternal(imageIndex, param, false);
>> Then the fix looks fine.
>>
>>> By others processXXX() do you mean processXXX() in other plugins or
>>> processXXX() in case of JPEG?
>>> Please clarify.
>> I meant only the code related to jpeg.
>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Sergey Bylokhov
>>> Sent: Wednesday, August 31, 2016 4:22 PM
>>> To: Jayathirth D V; Philip Race
>>> Cc: 2d-dev
>>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort()
>>> method does not work when called inside imageStarted for PNG
>>>
>>> I have only one question: should we call the new
>>> clearNativeReadAbortFlag(and probably the others processXXX()) under
>>> the ThreadLock?
>>>
>>> On 31.08.16 13:07, Jayathirth D V wrote:
>>>> Hi Sergey,
>>>>
>>>> Thanks for the clarification.
>>>> I have updated the test case to use Files.delete().
>>>> Please find updated webrev for review:
>>>> http://cr.openjdk.java.net/~jdv/4924727/webrev.02/
>>>>
>>>> Regards,
>>>> Jay
>>>>
>>>> -----Original Message-----
>>>> From: Sergey Bylokhov
>>>> Sent: Monday, August 29, 2016 8:52 PM
>>>> To: Jayathirth D V
>>>> Cc: 2d-dev
>>>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort()
>>>> method does not work when called inside imageStarted for PNG
>>>>
>>>> On 29.08.16 18:07, Jayathirth D V wrote:
>>>>> Hi Sergey,
>>>>>
>>>>> I am not getting the usage of Files.delete() from its
>>>>> specification. Can you please elaborate what special case it will
>>>>> handle in my test case?
>>>>> I am creating temporary file separately for all the readers and
>>>>> deleting them.
>>>> Files.delete() will throw an exception if the file cannot be
>>>> deleted, and File.delete() will return false in such case.
>>>>
>>>> Also I am closing the ImageInputStream associated after read operation.
>>>>
>>>> But plugin itself can leak some streams and lock a temporary file,
>>>> so
>>>> Files.delete() will catch this.
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Sergey Bylokhov
>>>>> Sent: Monday, August 29, 2016 8:25 PM
>>>>> To: Jayathirth D V; Philip Race
>>>>> Cc: Prasanta Sadhukhan; 2d-dev
>>>>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort()
>>>>> method does not work when called inside imageStarted for PNG
>>>>>
>>>>> Hi, Jay.
>>>>> Please delete the temporary file via Files.delete(), which will
>>>>> throw an exception if the file is locked by some reader.
>>>>>
>>>>> On 29.08.16 11:42, Jayathirth D V wrote:
>>>>>> Hi Phil& Sergey,
>>>>>>
>>>>>> Thanks for your inputs.
>>>>>>
>>>>>> I have verified reader.abort() request for IIOReadProgressListener
>>>>>> for all available plugins.
>>>>>>
>>>>>> Apart from PNG although all readers were able to abort read when
>>>>>> we call reader.abort() from IIOReadProgressListener callbacks,
>>>>>> they were not calling processReadAborted() right after
>>>>>> IIOReadProgressListener callbacks. So I have made changes for the
>>>>>> same.
>>>>>>
>>>>>> And in some readers before every read call they were not calling
>>>>>> clearAbortRequest(), which is important because if we use same
>>>>>> reader for another read() call it will be invalid unless we clear
>>>>>> previous abort request.
>>>>>>
>>>>>> In case of JPEG since we are using native IJG library we need to
>>>>>> update abortFlag present in imageioJPEG.c before every call as we
>>>>>> are doing for other readers using clearAbortRequest().
>>>>>>
>>>>>> Since this has native and make changes I have verified changes
>>>>>> through JPRT also which is successfully building on all platforms
>>>>>> (http://scaaa637.us.oracle.com//archive/2016/08/2016-08-29-065104.jay.
>>>>>>
>>>>>> client_commit//JobStatus.txt )
>>>>>>
>>>>>> Please find updated webrev for review:
>>>>>> http://cr.openjdk.java.net/~jdv/4924727/webrev.01/
>>>>>>
>>>>>> I noticed that in case on WBMP I was not getting ImageReader
>>>>>> object to call setInput() in test case to verify the behavior of
>>>>>> reader.abort(). So I have created separate bug for the same
>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8164930 ). And in case
>>>>>> of WBMP we already have clearAbortRequest() call and also we are
>>>>>> returning from IIOReadProgressListener callbacks properly, only
>>>>>> thing here is we are not returning right after callbacks as we
>>>>>> have updated other plugins.
>>>>>>
>>>>>> I want to verify writer plugins in separate bug as we already have
>>>>>> lot of changes in this bug. So I have created
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8164931 and will be
>>>>>> working on this bug.
>>>>>>
>>>>>> Thanks,
>>>>>> Jay
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Phil Race
>>>>>> Sent: Thursday, August 18, 2016 1:42 AM
>>>>>> To: Sergey Bylokhov
>>>>>> Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev
>>>>>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort()
>>>>>> method does not work when called inside imageStarted for PNG
>>>>>>
>>>>>> I think we can
>>>>>> - get all plugins,and for each
>>>>>> - write a file in that format
>>>>>> - read it back and apply the test
>>>>>>
>>>>>> It is also worth verifying that the writer abort checks are in
>>>>>> sync with the reader aborts, ie happen at such equivalent points
>>>>>> as might exist.
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 08/15/2016 11:30 AM, Sergey Bylokhov wrote:
>>>>>>> Is it possible to unify the test for all our plugins? I assume
>>>>>>> they should work in the same way. I am not sure but probably the
>>>>>>> image can be generated at runtime?
>>>>>>>
>>>>>>> On 11.08.16 21:59, Jayathirth D V wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Please review the following fix in JDK9 at your convenience:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-4924727
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Webrev : http://cr.openjdk.java.net/~jdv/4924727/webrev.00/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Issue : When we issue ImageReader.abort() in
>>>>>>>> IIOReadProgressListener.imageStarted(), reading is not aborted
>>>>>>>> and it is continued.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Root cause : After IIOReadProgressListener.imageStarted() call
>>>>>>>> in PNGImageReader.java when we enter decodeImage() we call
>>>>>>>> clearAbortRequest() which will clear the abort request from
>>>>>>>> IIOReadProgressListener.imageStarted().
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Solution : clearAbortRequest() documentation mentions that it
>>>>>>>> should be called before reading of image starts, so it should be
>>>>>>>> called before IIOReadProgressListener.imageStarted()(In
>>>>>>>> PNGImageReader.java it is
>>>>>>>> processImageStarted(0) in readImage()). So moved
>>>>>>>> clearAbortRequest() call from decodeImage() to readImage(). Also
>>>>>>>> we should call
>>>>>>>> abortRequested() in PNGImageReader.java at places mapping to
>>>>>>>> IIOReadProgressListener and not randomly at end of functions or
>>>>>>>> at places related to IIOReadUpdateListener, updated the code for
>>>>>>>> the same.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Observation not related to this issue : We don't have call
>>>>>>>> similar to
>>>>>>>> IIOReadProgressListener.readAborted() in IIOReadUpdateListener,
>>>>>>>> but user can call ImageReader.abort() from IIOReadUpdateListener
>>>>>>>> methods.
>>>>>>>> Is there a need to add similar method in IIOReadUpdateListener?
>>>>>>>> Any inputs on this also would be helpful.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Jay
>>>>>>>>
>>>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>>>
>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>
More information about the 2d-dev
mailing list