[OpenJDK 2D-Dev] RFR: 8145776: [TEST] add a test checking multipage tiff creation

Thu Jan 14 11:41:02 UTC 2016

On 14/01/16 13:41, Alexander Stepanov wrote:
> Hello Sergey,
>
> In such a case I prefer to have a separate need-test bug to cover all
> the formats, please see
> https://bugs.openjdk.java.net/browse/JDK-8147082

ok, thanks. looks fine.

>
> This test was planned to cover tiff functionality (in the context of JEP
> 262).
>
> Thanks,
> Alexander
>
> On 1/13/2016 8:10 PM, Sergey Bylokhov wrote:
>> On 12/01/16 19:53, Alexander Stepanov wrote:
>>> So it seems the test logic could be used "as is" for the tiff format
>>> only (as intended), and I'd like to keep it without sizable changes.
>>
>> yes but in past we had a situations when such assumption was broken in
>> some new writers/readers: JDK-8146887, JDK-8144245, JDK-8145240. And
>> the bugs were uncovered because the tests verify all installed providers.
>>
>>>
>>> Regards,
>>> Alexander
>>>
>>> On 1/12/2016 6:18 PM, Alexander Stepanov wrote:
>>>> In such a case (canWriteSequence() && canWriteRasters()), I guess, we
>>>> return to the sole TIFF :)
>>>>
>>>> On 1/12/2016 6:14 PM, Sergey Bylokhov wrote:
>>>>> On 12/01/16 18:08, Alexander Stepanov wrote:
>>>>>>  > applicable to all image types which canWriteSequence=true?
>>>>>> I'm not sure. E.g., for "gif" canWriteSequence() returns "true", but
>>>>>> UnsupportedOperationException is thrown when calling writeInsert() (I
>>>>>> have to double-check).
>>>>>
>>>>> canWriteRasters should be checked also.
>>>>>
>>>>>>
>>>>>> (moreover, not sure if the gif created in such a way could be called
>>>>>> "multipage" in the same sense as tiff).
>>>>>>
>>>>>>  > "import javax.imageio.plugins.tiff.*" is unnecessary
>>>>>> yes, it should be removed, thanks.
>>>>>>
>>>>>> Regards,
>>>>>> Alexander
>>>>>>
>>>>>> On 1/12/2016 5:36 PM, Sergey Bylokhov wrote:
>>>>>>> Since "import javax.imageio.plugins.tiff.*" is unnecessary in the
>>>>>>> test, i guess it is applicable to all image types which
>>>>>>> canWriteSequence=true?
>>>>>>>
>>>>>>> On 11/01/16 14:49, Alexander Stepanov wrote:
>>>>>>>> Hello Semyon,
>>>>>>>>
>>>>>>>> Thank you, that eliminates the question with the internal package.
>>>>>>>> Probably Brian meant the same (but I just didn't understand this at
>>>>>>>> first).
>>>>>>>>
>>>>>>>> Please review the updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~avstepan/8145776/webrev.02/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexander
>>>>>>>>
>>>>>>>> On 1/11/2016 11:59 AM, Semyon Sadetsky wrote:
>>>>>>>>> Hello Alexander,
>>>>>>>>>
>>>>>>>>> Why don't use the reader method analogue for writer? Then the test
>>>>>>>>> could use the public API which is preferable.
>>>>>>>>>
>>>>>>>>> Just a cosmetic note. Lines should be wrapped at 80 position.
>>>>>>>>>
>>>>>>>>> --Semyon
>>>>>>>>>
>>>>>>>>> On 12/29/2015 3:44 PM, Alexander Stepanov wrote:
>>>>>>>>>> Hello Brian,
>>>>>>>>>>
>>>>>>>>>> Thank you for the notes, please see the updated webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~yan/8145776/webrev.01/
>>>>>>>>>>
>>>>>>>>>> 1., 3. - fixed
>>>>>>>>>> WRT 2.: this import is necessary to use TIFFImageWriter,
>>>>>>>>>> TIFFImageWriterSpi. As it follows from
>>>>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/07ae3247e988, the
>>>>>>>>>> majority of new classes (excepting tag sets) were added to
>>>>>>>>>> com.sun.imageio.plugins.tiff. Do you mean these classes would be
>>>>>>>>>> moved to javax/imageio soon? (please note also a @module tag
>>>>>>>>>> added
>>>>>>>>>> for compatibility with modular java).
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Alexander
>>>>>>>>>>
>>>>>>>>>> On 12/29/2015 2:12 AM, Brian Burkhalter wrote:
>>>>>>>>>>> Hello Alexander,
>>>>>>>>>>>
>>>>>>>>>>> A few comments.
>>>>>>>>>>>
>>>>>>>>>>> 1) Lines 53,59: By convention it is better to put constants in
>>>>>>>>>>> upper
>>>>>>>>>>> case, e.g., NUM_IMAGES and BLACK_SIZE.
>>>>>>>>>>>
>>>>>>>>>>> 2) Line 45, 102: The test references the internal package
>>>>>>>>>>> com.sun.imageio.plugins.tiff. In general it is better to use the
>>>>>>>>>>> public packages.
>>>>>>>>>>>
>>>>>>>>>>> 3) Line 69: If the test fails, it would be good to know the
>>>>>>>>>>> random
>>>>>>>>>>> seed. It would also be good to be able to set the seed when
>>>>>>>>>>> running
>>>>>>>>>>> the test. The approach taken in core libraries may be seen in
>>>>>>>>>>> test/java/math/BigInteger/BigIntegerTest.java. The
>>>>>>>>>>> jdk.testlibrary.RandomFactory class is used to create the Random
>>>>>>>>>>> instance and to obtain any seed provided via Java properties.
>>>>>>>>>>> This
>>>>>>>>>>> requires the test tags “@library /lib/testlibrary” and “@build
>>>>>>>>>>> jdk.testlibrary.*” and it is also good to add “@key randomness.”
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>>
>>>>>>>>>>> Brian
>>>>>>>>>>>
>>>>>>>>>>> On Dec 25, 2015, at 5:10 AM, Alexander Stepanov
>>>>>>>>>>> <alexander.v.stepanov at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Could you please review the following fix
>>>>>>>>>>>> http://cr.openjdk.java.net/~yan/8145776/webrev.00/
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eyan/8145776/webrev.00/>
>>>>>>>>>>>> for
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8145776
>>>>>>>>>>>>
>>>>>>>>>>>> Just a single test added checking the correctness of multi-page
>>>>>>>>>>>> TIFF image creation.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>

-- 
Best regards, Sergey.