118606 - support opentype format 12 CMAP table

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Shanjian Li]

Opentye specification (http://partners.adobe.com/asn/developer/opentype/main.html)
defined several new CMAP formats. One of them is format 12, which is standard 
CMAP for support of UCS4 characters. We only support format 4 for now. Format 12 
is needed to support non-BMP characters.

Comment 1

[Shanjian Li]

Attachment: proposed patch

Comment 2

[Shanjian Li]

frank, rbs, brian,
Could one of you code review my patch? 

Comment 3

[rbs]

+    PRUint32** extMap = new PRUint32*[EXTENDED_UNICODE_PLANES+1];
[...]
->   + ReadCMAPTableFormat12()
     + {
     +   for ( c = startCode; c <= endCode; ++c) {
     +   plane = c >> 16;
     +   if (!extMap[plane]) {
     +     extMap[plane] = new PRUint32[UCS2_MAP_LEN];
     +   [...]
     + }
[...]
+    delete extMap;

Looks like it is leaking here. Are you note deleting the extMap[planes] as well?

Comment 4

[Shanjian Li]

Attachment: updated patch fix leakage

Comment 5

[Shanjian Li]

Attachment: updated patch (fix leakage, follow argument name convention)

Comment 6

[Shanjian Li]

The latest patch fix the leakage pointed out by rbs (thanks!). I also renamed 
function parameters, which I planned but forgot. Other than that, all remains the 
same. 

Comment 7

[rbs]

... I was just about to ask why your even need to new extMap[] itself; if you
just need a temp, you could use "PRUint32* extMap[EXTENDED_UNICODE_PLANES+1]"
and pass &extMap to the other function, and then, only delete the allocated
inner planes once your are done.

Comment 8

[rbs]

r=rbs applies from now on.

Comment 9

[Shanjian Li]

Attachment: update remove unnecessary heap allocation

Comment 10

[Shanjian Li]

Yes, it does not make sense not to use stack for 17 int32. updated as such. 

Comment 11

[Shanjian Li]

cc to marc. 

Marc, could you sr?

Comment 12

[Marc Attinasi]

Comment on attachment 63907 [details] [diff] [review]
update remove unnecessary heap allocation

sr=attinasi

Comment 13

[rbs]

Spotted at something:

+nsresult 
+ReadCMAPTableFormat4(PRUint8* aBuf, PRInt32 aLength, PRUint32* aMap, PRUint8* 
aIsSpace) 
+{
+  PRUint8* p;
+  PRUint8* end = aBuf + aLength;
+  PRUint32 i;
+
+  p = aBuf;
+  PRUint16 format = GET_SHORT(p);
+  if (format != eTTFormat4SegmentMappingToDeltaValues) {
+    nsMemory::Free(aBuf);
+    return nsnull; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 0 is NS_OK as nsresult! 
+  }
[...]

============
+  else if (eTTFormat4SegmentMappingToDeltaValues == keepFormat) {
+    PRUint32 maxGlyph;
+    PRUint8* isSpace = GetSpaces(aDC, &maxGlyph);
+    if (isSpace) {
+      res = ReadCMAPTableFormat4(buf+keepOffset, len-keepOffset, map, isSpace);
+      nsMemory::Free(isSpace);
+      if (NS_SUCCEEDED(res))
+        ccmap = MapToCCMap(map);
+      aCharset = DEFAULT_CHARSET;
+      aFontType = eFontType_Unicode;
     }
   }
see the other test for |if (format != eTTFormat4SegmentMappingToDeltaValues)| is 
an early test, you could probably move that out and put it soon after this test 
for |if (eTTFormat4SegmentMappingToDeltaValues == keepFormat)|.

Comment 14

[Shanjian Li]

rbs,
Those 2 values (keepFormat and format) are got from different places. The second 
instance is for verification purpose only. If it fails, it means the font file 
must be corrupted. IMO the verification can be omitted, since OS must have done it 
already. I would like to remove it, what do you think?

Comment 15

[rbs]

Having the test is good since it makes the code more robust. Surprises sometimes
arise with fonts.

Comment 16

[Shanjian Li]

fix checked in. 

Comment 17

[rbs]

I had a look at your checkin and noted that the problem that I reported was
still there. I wasn't clear enough in my comments.

What I meant is that your have the function |nsresult| ReadCMAPTableFormat4()
that is returning nsnull in case of error, and you are testing if NS_SUCCEEDED()
to do other things later on. In case of an error, there is a problem because
nsnull is actuallay 0, i.e. NS_OK if seen as an nsresult...

What I suggested was to remove the test from ReadCMAPTableFormat[4|12] and do
something like:

+  else if (eTTFormat4SegmentMappingToDeltaValues == keepFormat) {
>>>>>PRUint16 format = GET_SHORT(p);
>>>>>if (format != eTTFormat4SegmentMappingToDeltaValues) {
  +    PRUint32 maxGlyph;
  +    PRUint8* isSpace = GetSpaces(aDC, &maxGlyph);
[...]
>>>>>}
 }

with this, ReadCMAPTableFormat[4|12] could simply be |void|.

Comment 18

[Shanjian Li]

rbs, you are right. I will take care of this in other bug. 

Comment 19

[rbs]

Attachment: patch to fix the nsnull problem

I was waiting on this bug before doing my alloc-only-if-needed changes. So I
have iterated on your patch to finish off the nsnull problem since it hindered
what I am thinking of doing. If you are okay with my above changes, I will go
on from there to do the alloc-only-if-needed changes and attach the final patch
on 
bug 117637.

Comment 20

[Shanjian Li]

rbs, you patch looks fine. (Thanks for taking care of my mess.)

Comment 21

[Frank Tang]

reopen to get rbs's patch review and check in. 

Comment 22

[Shanjian Li]

rbs's patch will be checked in with 117637.