[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort() method does not work when called inside im
From: Philip Race <philip.race () oracle ! com>
Date: 2016-08-31 23:17:40
Message-ID: 57C76594.2000504 () oracle ! com
[Download RAW message or body]
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.
>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic