Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v

[Vadim Pakhnushev]

That's what warning said: "place parentheses *around the assignment *to 
silence this warning"


Vadim

On 02.08.2016 8:48, Prasanta Sadhukhan wrote:

+1. Only one thing,

mlib_c_ImageCopy.c#185 do we really need that extra parentheses,
((j = (w & 1))). I guess this should just do if (j = (w & 1)), isn't it? Regards
Prasanta
On 8/1/2016 10:43 PM, Phil Race wrote:

Looking for another "+1" ...

-phil.

On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:

Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as 
separate assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the 
loop variable.
It looks like there's an error in parentheses and it should 
instead look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it 
without trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* 
reach

the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; 
but all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  
while(0) stuff was to enable the macro calls in conditions like if 
(foo) CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro 
definition this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, 
maybe all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an 
assignment as a condition without parentheses 
[-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1) \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a 
statement

Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v

[Prasanta Sadhukhan]

+1. Only one thing,

mlib_c_ImageCopy.c#185 do we really need that extra parentheses,

((j = (w & 1))). I guess this should just do if (j = (w & 1)), isn't it? Regards
Prasanta

On 8/1/2016 10:43 PM, Phil Race wrote:

Looking for another "+1" ...

-phil.

On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:

Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as 
separate assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the 
loop variable.
It looks like there's an error in parentheses and it should 
instead look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it 
without trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* 
reach

the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; 
but all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  while(0) 
stuff was to enable the macro calls in conditions like if (foo) 
CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro 
definition this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, 
maybe all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an 
assignment as a condition without parentheses 
[-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not  help sin

Re: [OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer

[Jim Graham]

Hi Laurent,

A tip on webrevs, if you supply a file of filenames then you can tell it 
to diff a file with a name change against its former name by using 2 
filenames on the same line, as in:



filetodiff1.java
filetodiff2.java
filetodiff3.java filetodiff3oldname.java
filetodiff4.java


In Renderer.java, you create the alphaLine and blkFlags refs as Clean, 
but then you always put them back using indices of (0, 0) so they will 
never actually be cleaned - is there a reason you don't just use a dirty 
ref there?


Other than that question, I don't see any problems with the fix...

...jim

On 08/01/2016 02:17 PM, Laurent Bourgès wrote:

Hi Jim,

Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159638.1/

Changes:
- merge Clean/Dirty Xxx ArrayCache using the flag 'clean' to indicate if
the cache is clean or dirty + the putArray method always performs the
array cleanup
- ArrayCache renamed to ArrayCacheConst + simplified thresholds

More comments, below:

2016-07-21 23:41 GMT+02:00 Jim Graham mailto:james.gra...@oracle.com>>:


On 07/21/2016 06:56 AM, Laurent Bourgès wrote:

I don't have any issues with those numbers, but the way that
they
are calculated makes it look like they are supposed to be unique
numbers and yet through the obscurity of the loops used to
populate
the sizes they just end up all being the same numbers - it
makes me
wonder if there was a mistake in there or not...?


Initially these values have different values / meanings but it
can be
simplified now.


To be clear, I wasn't complaining about the multitude of thresholds,
but rather that the way they were apportioned - sort of as a side
effect of the computations in a loop - ended up accidentally
squashing a couple of them out of existence.  Another option would
be to make sure that the thresholds make sense, but keep all 4
threshold ranges, but you are the one who knows the details of how
these sizes impact performance...


Simplified a bit + fixed comments.



It feels odd to have all of the cache classes import the static
members of ArrayCache rather than just subclassing it. Is
there a
reason it wasn't just subclassed?


As all the members are static, I prefer having an explicit
static import
instead of subclasses.
Moreover, I deliberately avoided any class inheritance to avoid the
performance penalty of bimorphic / megamorphic method calls
(abstract or
overriden methods).


First, I would have expected MumbleArrayCache classes to be a
subclass of the ArrayCache class in the first place.  If it is not
going to be the base class then it should have the name
"ArrayCacheUtils" or "Const" or something.


Renamed to ArrayCacheConst.



Second, wildcard imports are recommended against.

Finally, if you are going to use static methods from another class I
would much rather see explicit naming rather than importing them.
While static imports work for methods as well as constants, they are
typically used for constants and it is confusing to apply them to
methods.


Fixed.


The only reason is performance: I want to ensure straightforward
method
calls ie no bimorphic or virtual calls to be inlined by hotspot.
Maybe
such fear or approach is too conservative i.e. the performance
penalty
is too small to consider such optimization. I made many tests
with jmh
(in june) but I agree the design can be simpler:
- abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract)
- Clean[Byte/Int/Float]ArrayCache and
Dirty[Byte/Int/Float]ArrayCache
implements properly the putArray method but also the createArray()
method (new array or Unsafe.allocateUninitializedArray)

I could try again but I prefer the current design as it is (overly)
strongly typed.
Please propose an alternative / simpler design !


That's something that can be investigated later as it would be a big
disruption for the current task.  Generally, though, as long as the
leaf classes are final and the cache classes are strongly typed then
HS should inline freely.


I adopted a simple boolean flag 'clean' to perform the optional cleanup
and renamed classes to Byte/Float/Int ArrayCache.



Also, since you never put the initial arrays, they aren't
automatically cleaned...?

Exactly: initial arrays are allocated by the Reference class and
directly used by callers (fill / clean) and the XxxArrayCache never
touch them.


What I was getting at was that this was potentially a bug?  If you
carry over use of an initial array in a clean setting without

Re: [OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer

[Laurent Bourgès]

Hi Jim,

Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159638.1/

Changes:
- merge Clean/Dirty Xxx ArrayCache using the flag 'clean' to indicate if
the cache is clean or dirty + the putArray method always performs the array
cleanup
- ArrayCache renamed to ArrayCacheConst + simplified thresholds

More comments, below:

2016-07-21 23:41 GMT+02:00 Jim Graham :

>
> On 07/21/2016 06:56 AM, Laurent Bourgès wrote:
>
>> I don't have any issues with those numbers, but the way that they
>> are calculated makes it look like they are supposed to be unique
>> numbers and yet through the obscurity of the loops used to populate
>> the sizes they just end up all being the same numbers - it makes me
>> wonder if there was a mistake in there or not...?
>>
>>
>> Initially these values have different values / meanings but it can be
>> simplified now.
>>
>
> To be clear, I wasn't complaining about the multitude of thresholds, but
> rather that the way they were apportioned - sort of as a side effect of the
> computations in a loop - ended up accidentally squashing a couple of them
> out of existence.  Another option would be to make sure that the thresholds
> make sense, but keep all 4 threshold ranges, but you are the one who knows
> the details of how these sizes impact performance...
>

Simplified a bit + fixed comments.


>
> It feels odd to have all of the cache classes import the static
>> members of ArrayCache rather than just subclassing it. Is there a
>> reason it wasn't just subclassed?
>>
>>
>> As all the members are static, I prefer having an explicit static import
>> instead of subclasses.
>> Moreover, I deliberately avoided any class inheritance to avoid the
>> performance penalty of bimorphic / megamorphic method calls (abstract or
>> overriden methods).
>>
>
> First, I would have expected MumbleArrayCache classes to be a subclass of
> the ArrayCache class in the first place.  If it is not going to be the base
> class then it should have the name "ArrayCacheUtils" or "Const" or
> something.
>

Renamed to ArrayCacheConst.


>
> Second, wildcard imports are recommended against.
>
> Finally, if you are going to use static methods from another class I would
> much rather see explicit naming rather than importing them.  While static
> imports work for methods as well as constants, they are typically used for
> constants and it is confusing to apply them to methods.
>

Fixed.


> The only reason is performance: I want to ensure straightforward method
>> calls ie no bimorphic or virtual calls to be inlined by hotspot. Maybe
>> such fear or approach is too conservative i.e. the performance penalty
>> is too small to consider such optimization. I made many tests with jmh
>> (in june) but I agree the design can be simpler:
>> - abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract)
>> - Clean[Byte/Int/Float]ArrayCache and Dirty[Byte/Int/Float]ArrayCache
>> implements properly the putArray method but also the createArray()
>> method (new array or Unsafe.allocateUninitializedArray)
>>
>> I could try again but I prefer the current design as it is (overly)
>> strongly typed.
>> Please propose an alternative / simpler design !
>>
>
> That's something that can be investigated later as it would be a big
> disruption for the current task.  Generally, though, as long as the leaf
> classes are final and the cache classes are strongly typed then HS should
> inline freely.
>

I adopted a simple boolean flag 'clean' to perform the optional cleanup and
renamed classes to Byte/Float/Int ArrayCache.


>
> Also, since you never put the initial arrays, they aren't
>> automatically cleaned...?
>>
>> Exactly: initial arrays are allocated by the Reference class and
>> directly used by callers (fill / clean) and the XxxArrayCache never
>> touch them.
>>
>
> What I was getting at was that this was potentially a bug?  If you carry
> over use of an initial array in a clean setting without calling to the
> cache object, then who clears the used portion?
>
>  All cases are manually cleaned in MarlinCache.resetTileLine(),
>> Renderer.dispose() and MarlinCache.copyAARowNoRLE...() for alphaLine and
>> blkFlags arrays with several cleanup patterns (optimized and
>> performance-critical).
>>
>
> I see.  Is this really a noticeable performance issue to rely on the cache
> to do the cleaning and have much more readable code?
>

I adopted your proposal and simplified the code in Renderer: the
Reference.putArray() clears the array portion if needed in all cases.
The performance impact is very low: only 3% when the shape count is huge.


Cheers,
Laurent

Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8160943 : skipImage() in JPEGImageReader class throws IIOException if we have gaps between markers in Jpeg image

[Jim Graham]

Hi Jay,

This looks good...

...jim

On 8/1/16 1:26 AM, Jayathirth D V wrote:

Hi,



Please review the following fix in JDK9 at your convenience:



Bug :  https://bugs.openjdk.java.net/browse/JDK-8160943

Webrev : http://cr.openjdk.java.net/~jdv/8160943/webrev.00/



Root cause : While fixing JDK-8152672 we considered that there can be no data 
in Jpeg image which can be like “FF FF”
and we threw IIOException if we find data as “FF FF” and considered it as 
invalid marker.



Solution : According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf there can 
be gaps between markers in Jpeg image and
it can contain fill bits like “X FF”.

In this case if a fill bit is followed by any valid maker in Jpeg it will 
contain sequence of data which will be “FF
FF”. So we should not consider “FF FF” as invalid marker and just parse through 
it. Added check to consider “FF FF” as
normal data in Jpeg.



Thanks,

Jay

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java

[Brian Burkhalter]

+1 for the review with a preference for Vadim’s suggestion.

Brian

On Aug 1, 2016, at 10:15 AM, Phil Race  wrote:

> That would be fine too. The main thing was getting rid of Class.forName
> 
> -phil.
> 
> On 08/01/2016 10:19 AM, Vadim Pakhnushev wrote:
>> Why not just registry.getServiceProviders(ImageReaderSpi.class, ?
>> 
>> Vadim
>> 
>> On 01.08.2016 19:13, Jayathirth D V wrote:
>>> Hi,
>>>  
>>> Please review the following fix in JDK9 at your convenience :
>>>  
>>> Bug :https://bugs.openjdk.java.net/browse/JDK-8160736
>>> Webrev :http://cr.openjdk.java.net/~jdv/8160736/webrev.00/
>>>  
>>> Root Cause : KSS tool has detected usage of class.forName where it can be 
>>> avoided.
>>> Solution : Use  instead of using class.forName.
>>>  
>>> Thanks,
>>> Jay
>> 
> 
> 

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java

[Phil Race]

That would be fine too. The main thing was getting rid of Class.forName

-phil.

On 08/01/2016 10:19 AM, Vadim Pakhnushev wrote:

Why not just registry.getServiceProviders(ImageReaderSpi.class, ?

Vadim

On 01.08.2016 19:13, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 at your convenience :

Bug : https://bugs.openjdk.java.net/browse/JDK-8160736

Webrev : http://cr.openjdk.java.net/~jdv/8160736/webrev.00/ 



Root Cause : KSS tool has detected usage of class.forName where it 
can be avoided.


Solution : Use  instead of using class.forName.

Thanks,

Jay

Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v

[Phil Race]

Looking for another "+1" ...

-phil.

On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:

Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as 
separate assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the 
loop variable.
It looks like there's an error in parentheses and it should 
instead look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it 
without trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* reach
the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; but 
all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  while(0) 
stuff was to enable the macro calls in conditions like if (foo) 
CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro definition 
this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, 
maybe all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an 
assignment as a condition without parentheses 
[-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not help since
then it said "loop terminator not reached - other cases we have
like this have at least a condition in the macro.

-phil.

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java

[Vadim Pakhnushev]

Why not just registry.getServiceProviders(ImageReaderSpi.class, ?

Vadim

On 01.08.2016 19:13, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 at your convenience :

Bug : https://bugs.openjdk.java.net/browse/JDK-8160736

Webrev : http://cr.openjdk.java.net/~jdv/8160736/webrev.00/ 



Root Cause : KSS tool has detected usage of class.forName where it can 
be avoided.


Solution : Use  instead of using class.forName.

Thanks,

Jay

[OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java

[Jayathirth D V]

Hi,

 

Please review the following fix in JDK9 at your convenience :

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8160736 

Webrev : http://cr.openjdk.java.net/~jdv/8160736/webrev.00/ 

 

Root Cause : KSS tool has detected usage of class.forName where it can be 
avoided.

Solution : Use  instead of using class.forName.

 

Thanks,

Jay

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java

[Philip Race]

+1

-phil

On 8/1/16, 9:13 AM, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 at your convenience :

Bug : https://bugs.openjdk.java.net/browse/JDK-8160736

Webrev : http://cr.openjdk.java.net/~jdv/8160736/webrev.00/ 



Root Cause : KSS tool has detected usage of class.forName where it can 
be avoided.


Solution : Use  instead of using class.forName.

Thanks,

Jay

Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8160943 : skipImage() in JPEGImageReader class throws IIOException if we have gaps between markers in Jpeg image

[Jayathirth D V]

Hi Phil,

 

Thanks for your review.

I have run all jtreg tests under imageio and there are no side effects.

 

Thanks,

Jay

 

From: Philip Race 
Sent: Monday, August 01, 2016 7:22 PM
To: Jayathirth D V
Cc: Jim Graham; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8160943 : skipImage() in 
JPEGImageReader class throws IIOException if we have gaps between markers in 
Jpeg image

 

+1 - assuming you have run the jtreg tests for imageio ..

-phil.

On 8/1/16, 1:26 AM, Jayathirth D V wrote: 

Hi,

 

Please review the following fix in JDK9 at your convenience:

 

Bug :  https://bugs.openjdk.java.net/browse/JDK-8160943

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8160943/webrev.00/"http://cr.openjdk.java.net/~jdv/8160943/webrev.00/
 

 

Root cause : While fixing JDK-8152672 we considered that there can be no data 
in Jpeg image which can be like "FF FF" and we threw IIOException if we find 
data as "FF FF" and considered it as invalid marker.

 

Solution : According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf there can 
be gaps between markers in Jpeg image and it can contain fill bits like "X FF".

In this case if a fill bit is followed by any valid maker in Jpeg it will 
contain sequence of data which will be "FF FF". So we should not consider "FF 
FF" as invalid marker and just parse through it. Added check to consider "FF 
FF" as normal data in Jpeg.

 

Thanks,

Jay

Re: [OpenJDK 2D-Dev] Fwd: JDK bug 8078516

[Philip Race]

Transparency of the surface has always disabled LCD sub-pixel in Java 2D.
Same happens on windows. The labels at the bottom
are drawn in greyscale.

-phil.

On 8/1/16, 7:55 AM, Torgeir Veimo wrote:
Ok, am asking since subpixel rendering works and is usable in most 
situations, except for the odd corner case when drawing over existing 
content in transparent surfaces.


See eg. this screenshot of netbeans, with nice subpixel rendered text 
in the editor, while the labels on the buttons at the bottom have 
artifacts since they for some reason are drawn twice.


Inline images 1

On 2 August 2016 at 00:49, Kevin Rushforth > wrote:


Phil will be able to comment on this further. All I can confirm is
that 8078526 is closed as a duplicate of 8087201, so it is no
longer an open bug.

-- Kevin


Torgeir Veimo wrote:

I'm fairly sure it's the correct bug id, although I don't have
access to view it myself.

Bug 8078526 is about supporting (without artifacts) subpixel
rendering to transparent surfaces.

Bug 8087201 is about speeding up subpixel rendering.

On 2 August 2016 at 00:39, Kevin Rushforth
mailto:kevin.rushfo...@oracle.com>>
wrote:

Are you sure you have the right Bug ID? That bug (which is
not publicly visible), is closed as a duplicate of 8087201,
which was fixed in JDK 9 and 8u66 over a year ago.

-- Kevin



Torgeir Veimo wrote:

Is there any more work done on JDK bug 8078516?
-- 
-Tor




-- 
-Tor





--
-Tor

Re: [OpenJDK 2D-Dev] Fwd: JDK bug 8078516

[Torgeir Veimo]

Ok, am asking since subpixel rendering works and is usable in most
situations, except for the odd corner case when drawing over existing
content in transparent surfaces.

See eg. this screenshot of netbeans, with nice subpixel rendered text in
the editor, while the labels on the buttons at the bottom have artifacts
since they for some reason are drawn twice.

 [image: Inline images 1]

On 2 August 2016 at 00:49, Kevin Rushforth 
wrote:

> Phil will be able to comment on this further. All I can confirm is that
> 8078526 is closed as a duplicate of 8087201, so it is no longer an open bug.
>
> -- Kevin
>
>
> Torgeir Veimo wrote:
>
> I'm fairly sure it's the correct bug id, although I don't have access to
> view it myself.
>
> Bug 8078526 is about supporting (without artifacts) subpixel rendering to
> transparent surfaces.
>
> Bug 8087201 is about speeding up subpixel rendering.
>
> On 2 August 2016 at 00:39, Kevin Rushforth 
> wrote:
>
>> Are you sure you have the right Bug ID? That bug (which is not publicly
>> visible), is closed as a duplicate of 8087201, which was fixed in JDK 9 and
>> 8u66 over a year ago.
>>
>> -- Kevin
>>
>>
>>
>> Torgeir Veimo wrote:
>>
>>> Is there any more work done on JDK bug 8078516?
>>> --
>>> -Tor
>>
>>
>
> --
> -Tor
>
>


-- 
-Tor

Re: [OpenJDK 2D-Dev] Fwd: JDK bug 8078516

[Kevin Rushforth]

Phil will be able to comment on this further. All I can confirm is that 
8078526 is closed as a duplicate of 8087201, so it is no longer an open bug.


-- Kevin


Torgeir Veimo wrote:
I'm fairly sure it's the correct bug id, although I don't have access 
to view it myself. 

Bug 8078526 is about supporting (without artifacts) subpixel rendering 
to transparent surfaces. 


Bug 8087201 is about speeding up subpixel rendering.

On 2 August 2016 at 00:39, Kevin Rushforth > wrote:


Are you sure you have the right Bug ID? That bug (which is not
publicly visible), is closed as a duplicate of 8087201, which was
fixed in JDK 9 and 8u66 over a year ago.

-- Kevin



Torgeir Veimo wrote:

Is there any more work done on JDK bug 8078516?
-- 
-Tor




--
-Tor

[OpenJDK 2D-Dev] Fwd: JDK bug 8078516

[Torgeir Veimo]

I'm fairly sure it's the correct bug id, although I don't have access to
view it myself.

Bug 8078526 is about supporting (without artifacts) subpixel rendering to
transparent surfaces.

Bug 8087201 is about speeding up subpixel rendering.

On 2 August 2016 at 00:39, Kevin Rushforth 
wrote:

> Are you sure you have the right Bug ID? That bug (which is not publicly
> visible), is closed as a duplicate of 8087201, which was fixed in JDK 9 and
> 8u66 over a year ago.
>
> -- Kevin
>
>
>
> Torgeir Veimo wrote:
>
>> Is there any more work done on JDK bug 8078516?
>> --
>> -Tor
>
>

-- 
-Tor

Re: [OpenJDK 2D-Dev] JDK bug 8078516

[Kevin Rushforth]

Are you sure you have the right Bug ID? That bug (which is not publicly 
visible), is closed as a duplicate of 8087201, which was fixed in JDK 9 
and 8u66 over a year ago.


-- Kevin


Torgeir Veimo wrote:
Is there any more work done on JDK bug 8078516? 


--
-Tor

[OpenJDK 2D-Dev] JDK bug 8078516

[Torgeir Veimo]

Is there any more work done on JDK bug 8078516?

-- 
-Tor

Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8160943 : skipImage() in JPEGImageReader class throws IIOException if we have gaps between markers in Jpeg image

[Philip Race]

+1 - assuming you have run the jtreg tests for imageio ..

-phil.

On 8/1/16, 1:26 AM, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 at your convenience:

Bug : https://bugs.openjdk.java.net/browse/JDK-8160943

Webrev : http://cr.openjdk.java.net/~jdv/8160943/webrev.00/ 



Root cause : While fixing JDK-8152672 we considered that there can be 
no data in Jpeg image which can be like "FF FF" and we threw 
IIOException if we find data as "FF FF" and considered it as invalid 
marker.


Solution : According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf 
there can be gaps between markers in Jpeg image and it can contain 
fill bits like "X FF".


In this case if a fill bit is followed by any valid maker in Jpeg it 
will contain sequence of data which will be "FF FF". So we should not 
consider "FF FF" as invalid marker and just parse through it. Added 
check to consider "FF FF" as normal data in Jpeg.


Thanks,

Jay

Re: [OpenJDK 2D-Dev] [9] RFR JDK-6575247: Banner checkbox in PrinterJob print dialog doesn't work

[Philip Race]

Ok. +1

-phil

On 8/1/16, 2:06 AM, Prasanta Sadhukhan wrote:



On 7/28/2016 9:19 PM, Phil Race wrote:
right .. when we talked off-line yesterday I said that if IPP is 
reporting

a default we should honour it. The default of standard/on is only for
the case we do not use IPP or it reports nothing.

 if (noJobSheet) {
 885 pFlags |= NOSHEET;
 886 ncomps+=1;
887 } else {
888 ncomps+=1;
 889 }


 905 if ((pFlags & NOSHEET) != 0) {
 906 execCmd[n++] = "-o nobanner";
907 } else if (getPrintService().
908 isAttributeCategorySupported(JobSheets.class)) {
909 execCmd[n++] = "-o job-sheets=standard";
 910 }

So shouldn't the else at line 887 have the same condition
as at line 907 ?

Yes. Have modified the webrev to add the check
http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.04/


2750 JobSheets js = (JobSheets)asCurrent.get(jsCategory);
2751 if (js == null) {
2752 js = 
(JobSheets)psCurrent.getDefaultAttributeValue(jsCategory);

2753 if (js == null) {
2754 js = JobSheets.STANDARD;
2755 }

Please make sure this isn't causing a surprise on OSX or Windows ..
ie that we report correctly a value there so that "null" doesn't
end up causing us to print a banner page where we did not before.

Tested on osx and windows too where it behaves as it is now ie no 
banner page is printed by default.


Regards
Prasanta

-phil.

On 7/28/2016 2:51 AM, Prasanta Sadhukhan wrote:
The banner checkbox is not enabled initially as it calls 
getDefaultAttributeValue() and it seems printer is reporting the 
default job-sheet value as "none" so the below check is not satisfied.

cbJobSheets.setSelected(js != JobSheets.NONE);

When IPPPrintService#readIPPResponse() is called, it invokes /new 
AttributeClass(attribStr,   valTagByte,  outArray);

/and stored the value corresponding to each attribute.

I saw that IPP response has
*job-sheets-default => job-sheets-default *and*
**job-sheets-supported => job-sheets-supported*

When AttributeClass constructor is called with 
("job-sheets-default", 66, value) the value is
[ 0] = (byte) 4[ 1] = (byte) 110[ 2] = (byte) 111[ 3] = 
(byte) 110[ 4] = (byte) 101[ 5] = (byte) 4[ 6] = (byte) 
110[ 7] = (byte) 111[ 8] = (byte) 110[ 9] = (byte) 
101[10] = (byte) 2

which basically translates to '',n,o,n,e,'',n,o,n,e,''

so when ServiceDialog calls 
IPPPrintService#getDefaultAttributeValue(JobSheets)


if (attribClass != null &&
attribClass.getStringValue().equals("none")) {
return JobSheets.NONE;
} else {
return JobSheets.STANDARD;
}
--
we have attribClass.getStringValue() as "none" so default job sheet 
value is coming out to be NONE.
Since it is coming out from IPP response, I think we should not 
change default value of JobSheets to STANDARD. Do you think otherwise?


In the modified webrev, I have made the jobsheet default value 
STANDARD, only when IPP does not report any default value.


http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.03/

Tested in ubuntu & solaris11.

Regards
Prasanta
On 7/27/2016 7:20 PM, Philip Race wrote:

883 } else {
 884 Class[] supportedCats = getPrintService().
 885 getSupportedAttributeCategories();
 886 for (int i=0;ihttps://docs.oracle.com/javase/8/docs/api/javax/print/PrintService.html#isAttributeCategorySupported-java.lang.Class- 



I am also very confused about why you added JOBSHEET
which seems to duplicate the functionality of NOSHEET.

Also it seems to me like it was intentional that the banner page be
printed by default .. which is why the variable was called 
"*no*JobSheet ..

so as to over-ride that behaviour.

It is kind of what you'd want if you walk over to the shared 
printer in

your office to have a banner page which declares it as yours ...

So the checkbox should probably be enabled in that case.

Also you should verify that we report the default value for JobSheets
as being STANDARD, not NONE.


-phil.

On 7/27/16, 3:02 AM, Prasanta Sadhukhan wrote:
Modified webrev to take care of a problem in webrev.01 whereby 
banner page was getting printed by default.
Now, banner page will get printed only if the checkbox is checked 
in printer dialog.


http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.02/

Regards
Prasanta
On 7/22/2016 4:26 PM, Prasanta Sadhukhan wrote:

Hi Phil,

I have modified the code to check if job-sheets is supported and 
then only proceed to print the banner page.
Also, rectified the jobTitle and banner confusion by adding 
jobsheet check.
Also added the same code in UnixPrintJob even though I found its 
printExecCmd() does not get executed in linux and solaris
PSPrinterJob's printExecCmd() is executed instead. In mac, 
neither UnixPrinterJob#printExecCmd() nor 
PSPrinterJob#printExecCmd() gets executed.


Tested on u

Re: [OpenJDK 2D-Dev] [9] RFR JDK-6575247: Banner checkbox in PrinterJob print dialog doesn't work

[Prasanta Sadhukhan]

On 7/28/2016 9:19 PM, Phil Race wrote:
right .. when we talked off-line yesterday I said that if IPP is 
reporting

a default we should honour it. The default of standard/on is only for
the case we do not use IPP or it reports nothing.

 if (noJobSheet) {
 885 pFlags |= NOSHEET;
 886 ncomps+=1;
887 } else {
888 ncomps+=1;
 889 }


 905 if ((pFlags & NOSHEET) != 0) {
 906 execCmd[n++] = "-o nobanner";
907 } else if (getPrintService().
908 isAttributeCategorySupported(JobSheets.class)) {
909 execCmd[n++] = "-o job-sheets=standard";
 910 }

So shouldn't the else at line 887 have the same condition
as at line 907 ?

Yes. Have modified the webrev to add the check
http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.04/


2750 JobSheets js = (JobSheets)asCurrent.get(jsCategory);
2751 if (js == null) {
2752 js = 
(JobSheets)psCurrent.getDefaultAttributeValue(jsCategory);

2753 if (js == null) {
2754 js = JobSheets.STANDARD;
2755 }

Please make sure this isn't causing a surprise on OSX or Windows ..
ie that we report correctly a value there so that "null" doesn't
end up causing us to print a banner page where we did not before.

Tested on osx and windows too where it behaves as it is now ie no banner 
page is printed by default.


Regards
Prasanta

-phil.

On 7/28/2016 2:51 AM, Prasanta Sadhukhan wrote:
The banner checkbox is not enabled initially as it calls 
getDefaultAttributeValue() and it seems printer is reporting the 
default job-sheet value as "none" so the below check is not satisfied.

cbJobSheets.setSelected(js != JobSheets.NONE);

When IPPPrintService#readIPPResponse() is called, it invokes /new 
AttributeClass(attribStr,   valTagByte,  outArray);

/and stored the value corresponding to each attribute.

I saw that IPP response has
*job-sheets-default => job-sheets-default *and*
**job-sheets-supported => job-sheets-supported*

When AttributeClass constructor is called with ("job-sheets-default", 
66, value) the value is
[ 0] = (byte) 4[ 1] = (byte) 110[ 2] = (byte) 111[ 3] = 
(byte) 110[ 4] = (byte) 101[ 5] = (byte) 4[ 6] = (byte) 
110[ 7] = (byte) 111[ 8] = (byte) 110[ 9] = (byte) 101
[10] = (byte) 2

which basically translates to '',n,o,n,e,'',n,o,n,e,''

so when ServiceDialog calls 
IPPPrintService#getDefaultAttributeValue(JobSheets)


if (attribClass != null &&
attribClass.getStringValue().equals("none")) {
return JobSheets.NONE;
} else {
return JobSheets.STANDARD;
}
--
we have attribClass.getStringValue() as "none" so default job sheet 
value is coming out to be NONE.
Since it is coming out from IPP response, I think we should not 
change default value of JobSheets to STANDARD. Do you think otherwise?


In the modified webrev, I have made the jobsheet default value 
STANDARD, only when IPP does not report any default value.


http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.03/

Tested in ubuntu & solaris11.

Regards
Prasanta
On 7/27/2016 7:20 PM, Philip Race wrote:

883 } else {
 884 Class[] supportedCats = getPrintService().
 885 getSupportedAttributeCategories();
 886 for (int i=0;ihttps://docs.oracle.com/javase/8/docs/api/javax/print/PrintService.html#isAttributeCategorySupported-java.lang.Class- 



I am also very confused about why you added JOBSHEET
which seems to duplicate the functionality of NOSHEET.

Also it seems to me like it was intentional that the banner page be
printed by default .. which is why the variable was called 
"*no*JobSheet ..

so as to over-ride that behaviour.

It is kind of what you'd want if you walk over to the shared printer in
your office to have a banner page which declares it as yours ...

So the checkbox should probably be enabled in that case.

Also you should verify that we report the default value for JobSheets
as being STANDARD, not NONE.


-phil.

On 7/27/16, 3:02 AM, Prasanta Sadhukhan wrote:
Modified webrev to take care of a problem in webrev.01 whereby 
banner page was getting printed by default.
Now, banner page will get printed only if the checkbox is checked 
in printer dialog.


http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.02/

Regards
Prasanta
On 7/22/2016 4:26 PM, Prasanta Sadhukhan wrote:

Hi Phil,

I have modified the code to check if job-sheets is supported and 
then only proceed to print the banner page.
Also, rectified the jobTitle and banner confusion by adding 
jobsheet check.
Also added the same code in UnixPrintJob even though I found its 
printExecCmd() does not get executed in linux and solaris
PSPrinterJob's printExecCmd() is executed instead. In mac, neither 
UnixPrinterJob#printExecCmd() nor PSPrinterJob#printExecCmd() gets 
executed.


Tested on ubuntu and solaris 11 with the fix and banner page is 
printed wi

[OpenJDK 2D-Dev] [9] RFR: JDK-8160943 : skipImage() in JPEGImageReader class throws IIOException if we have gaps between markers in Jpeg image

[Jayathirth D V]

Hi,

 

Please review the following fix in JDK9 at your convenience:

 

Bug :  https://bugs.openjdk.java.net/browse/JDK-8160943

Webrev : http://cr.openjdk.java.net/~jdv/8160943/webrev.00/ 

 

Root cause : While fixing JDK-8152672 we considered that there can be no data 
in Jpeg image which can be like "FF FF" and we threw IIOException if we find 
data as "FF FF" and considered it as invalid marker.

 

Solution : According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf there can 
be gaps between markers in Jpeg image and it can contain fill bits like "X FF".

In this case if a fill bit is followed by any valid maker in Jpeg it will 
contain sequence of data which will be "FF FF". So we should not consider "FF 
FF" as invalid marker and just parse through it. Added check to consider "FF 
FF" as normal data in Jpeg.

 

Thanks,

Jay