Issue 1011403004: BitmapTextBatch and BitmapTextBlob

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1609 lines, -37 lines)			Patch
M	dm/DMSrcSink.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+13 lines, -0 lines	0 comments	Download
M	gyp/gpu.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+2 lines, -0 lines	0 comments	Download
M	include/core/SkPaint.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	include/core/SkTextBlob.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	include/gpu/GrContext.h	View	1 2 3	3 chunks	+3 lines, -0 lines	0 comments	Download
M	src/gpu/GrAADistanceFieldPathRenderer.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+2 lines, -6 lines	0 comments	Download
M	src/gpu/GrBatchAtlas.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	3 chunks	+4 lines, -0 lines	0 comments	Download
M	src/gpu/GrBatchAtlas.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	4 chunks	+5 lines, -2 lines	0 comments	Download
A	src/gpu/GrBatchFontCache.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+137 lines, -0 lines	0 comments	Download
A	src/gpu/GrBatchFontCache.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+314 lines, -0 lines	0 comments	Download
M	src/gpu/GrBitmapTextContext.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+121 lines, -0 lines	0 comments	Download
M	src/gpu/GrBitmapTextContext.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	7 chunks	+940 lines, -19 lines	0 comments	Download
M	src/gpu/GrContext.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	7 chunks	+14 lines, -0 lines	0 comments	Download
M	src/gpu/GrDistanceFieldTextContext.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/gpu/GrFontAtlasSizes.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+25 lines, -1 line	0 comments	Download
M	src/gpu/GrGlyph.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+10 lines, -6 lines	0 comments	Download
M	src/gpu/GrStencilAndCoverTextContext.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/gpu/GrTextContext.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	3 chunks	+8 lines, -3 lines	0 comments	Download
M	src/gpu/SkGpuDevice.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 28 (8 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

joshualitt

ptal. Let me know what needs comments. The only real remaining TODOs are using SkCache ...

5 years, 9 months ago (2015-03-23 15:49:42 UTC) #2

robertphillips

I see nothing incorrect but there is a lot of code here https://codereview.chromium.org/1011403004/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp ...

5 years, 9 months ago (2015-03-23 19:03:12 UTC) #4

joshualitt

feedback incorporated https://codereview.chromium.org/1011403004/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1011403004/diff/80001/dm/DMSrcSink.cpp#newcode205 dm/DMSrcSink.cpp:205: canvas->clipRect(kSKPViewport); On 2015/03/23 19:03:11, robertphillips wrote: > ...

5 years, 9 months ago (2015-03-23 19:56:09 UTC) #5

jvanverth1

Still need some more time to look at this -- looks good so far. A ...

5 years, 9 months ago (2015-03-23 20:37:39 UTC) #6

joshualitt

feedback inc https://codereview.chromium.org/1011403004/diff/120001/src/gpu/GrBatchAtlas.cpp File src/gpu/GrBatchAtlas.cpp (right): https://codereview.chromium.org/1011403004/diff/120001/src/gpu/GrBatchAtlas.cpp#newcode249 src/gpu/GrBatchAtlas.cpp:249: (*currPlot)->init(this, texture, id, 1, x, y, fPlotWidth, ...

5 years, 9 months ago (2015-03-24 14:33:56 UTC) #7

bsalomon

some initial comments/qs https://codereview.chromium.org/1011403004/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1011403004/diff/160001/dm/DMSrcSink.cpp#newcode207 dm/DMSrcSink.cpp:207: //#define DOUBLE_LOOP Let's make this real... ...

5 years, 9 months ago (2015-03-24 16:27:54 UTC) #9

joshualitt

Feedback incorporated. There is definitely one bug with this CL, namely we aren't refing plots ...

5 years, 9 months ago (2015-03-25 14:13:37 UTC) #10

Feedback incorporated.  There is definitely one bug with this CL, namely we
aren't refing plots in the atlas when we flush with a non-stale blob.  Robert
and I came up with a good fast solution, but its a large enough chunk of code
that I want to do it in a follow up.

https://codereview.chromium.org/1011403004/diff/160001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1011403004/diff/160001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:207: //#define DOUBLE_LOOP
On 2015/03/24 16:27:53, bsalomon wrote:
> Let's make this real... 
> 
> Maybe make dm take a param --numSkpDraws <n> and it draws the skps n times and
> outputs n images to gold with a tag 1..n for each. It shouldn't add much
> baselining overhead since they will usually be identical. We keep adding more
> caches to both the cpu and gpu backends, it'd be good to verify that we draw
the
> same (or nearly so) on a cache hit.
> 
> (separate CL, obviously)

Acknowledged.

https://codereview.chromium.org/1011403004/diff/160001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1011403004/diff/160001/include/gpu/GrContext....
include/gpu/GrContext.h:700: GrBatchFontCache*               fBatchFontCache;
On 2015/03/24 16:27:53, bsalomon wrote:
> Is this what gets replaced by skresourcecache?

This cache replaces fFontCache.  SkResourceCache replaces the hashmap on
GrBitmapTextContext

https://codereview.chromium.org/1011403004/diff/160001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/1011403004/diff/160001/src/core/SkTextBlob.cp...
src/core/SkTextBlob.cpp:127: uint32_t SkTextBlob::uniqueID() const {
On 2015/03/24 16:27:53, bsalomon wrote:
> Is it really worth populating this lazily? I know we don't in several other
> classes that maintain unique ids.

I have another CL out which addresses this

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.cpp:657: // TODO actually only invert if we don't
have RGBA
On 2015/03/24 16:27:53, bsalomon wrote:
> Is that not captured in this->usesLocalCoords()? If not, maybe we should
figure
> out how to capture it there? Hopefully that reflects whether any of the batch
> trackers were seeded with a pipeline info that indicated local coords were
> necessary.

You are correct about this->usesLocalCoords().  The problem is we should remove
shader stages when we have rgba text, but we currently don't do this.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.cpp:824: // we assume texture coords are the last
vertex attribute, this is a bit fragile.
On 2015/03/24 16:27:53, bsalomon wrote:
> why not just make the incoming ptr (vertices) already be offset to point to
the
> first texture coords? Then you know the next one is at vertices+vertexStride,
> and then vertices+2*vertexStride,...  If you do that, should rename the params
> to texCoords and texCoordsStride, or something along those lines. I think it'd
> simplify the ptr math in here a bit.

Acknowledged.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.cpp:849: intptr_t vertex =
reinterpret_cast<intptr_t>(vertices) + vertexStride * currVertex;
On 2015/03/24 16:27:53, bsalomon wrote:
> same... just take a ptr to the first vertex color... then this never has to
> change no matter how you rearrange the attrs.

Acknowledged.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:57: uint32_t fAtlasGeneration;
On 2015/03/24 16:27:53, bsalomon wrote:
> Does this mean that any change to the atlas invalidates the run even if plots
> associated with the run didn't change?

Correct.  Regenerating the texture coords doesn't seem to be *that* expensive,
so I'm worried having a ton of bookkeeping for more granularity here doesn't
make sense.  Its something we can explore though.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrGlyph.h
File src/gpu/GrGlyph.h (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrGlyph.h#newc...
src/gpu/GrGlyph.h:34: // TODO either plot or AtlasID will be valid, not both
On 2015/03/24 16:27:53, bsalomon wrote:
> Is this b/c we have both batch and nonbatch impls of text rendering?

yes

bsalomon

Seems ok if we land w/ that bug but behind the macro and fix before ...

5 years, 9 months ago (2015-03-25 14:19:15 UTC) #11

Seems ok if we land w/ that bug but behind the macro and fix before enabling.

https://codereview.chromium.org/1011403004/diff/160001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1011403004/diff/160001/include/gpu/GrContext....
include/gpu/GrContext.h:700: GrBatchFontCache*               fBatchFontCache;
On 2015/03/25 14:13:37, joshualitt wrote:
> On 2015/03/24 16:27:53, bsalomon wrote:
> > Is this what gets replaced by skresourcecache?
> 
> This cache replaces fFontCache.  SkResourceCache replaces the hashmap on
> GrBitmapTextContext

Ah, ok. Haven't looked at this yet.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.cpp:657: // TODO actually only invert if we don't
have RGBA
On 2015/03/25 14:13:37, joshualitt wrote:
> On 2015/03/24 16:27:53, bsalomon wrote:
> > Is that not captured in this->usesLocalCoords()? If not, maybe we should
> figure
> > out how to capture it there? Hopefully that reflects whether any of the
batch
> > trackers were seeded with a pipeline info that indicated local coords were
> > necessary.
> 
> You are correct about this->usesLocalCoords().  The problem is we should
remove
> shader stages when we have rgba text, but we currently don't do this.

You mean if there is a SkShader we will add a corresponding GrFP to the
GrPaint/GrPB?

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:57: uint32_t fAtlasGeneration;
On 2015/03/25 14:13:37, joshualitt wrote:
> On 2015/03/24 16:27:53, bsalomon wrote:
> > Does this mean that any change to the atlas invalidates the run even if
plots
> > associated with the run didn't change?
> 
> Correct.  Regenerating the texture coords doesn't seem to be *that* expensive,
> so I'm worried having a ton of bookkeeping for more granularity here doesn't
> make sense.  Its something we can explore though.

Gotcha, just wanted to make sure I understood it correctly.

bsalomon

Some more comments, mostly about the cache. I haven't read in detail GrBitmapTextContext.cpp. Leaving that ...

5 years, 9 months ago (2015-03-26 17:06:56 UTC) #13

joshualitt

feedback incorporated. https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextContext.cpp File src/gpu/GrBitmapTextContext.cpp (right): https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextContext.cpp#newcode657 src/gpu/GrBitmapTextContext.cpp:657: // TODO actually only invert if we ...

5 years, 9 months ago (2015-03-26 18:21:43 UTC) #14

feedback incorporated.

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/1011403004/diff/160001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.cpp:657: // TODO actually only invert if we don't
have RGBA
On 2015/03/25 14:19:15, bsalomon wrote:
> On 2015/03/25 14:13:37, joshualitt wrote:
> > On 2015/03/24 16:27:53, bsalomon wrote:
> > > Is that not captured in this->usesLocalCoords()? If not, maybe we should
> > figure
> > > out how to capture it there? Hopefully that reflects whether any of the
> batch
> > > trackers were seeded with a pipeline info that indicated local coords were
> > > necessary.
> > 
> > You are correct about this->usesLocalCoords().  The problem is we should
> remove
> > shader stages when we have rgba text, but we currently don't do this.
> 
> You mean if there is a SkShader we will add a corresponding GrFP to the
> GrPaint/GrPB?

correct

https://codereview.chromium.org/1011403004/diff/240001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1011403004/diff/240001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:206: // Testing TextBlob batching requires that we see
individual text blobs more than once
On 2015/03/26 17:06:55, bsalomon wrote:
> revert before landing?

I'll fix this when I land the better fix to DM.  I'd rather leave this in so we
have some manual way to test this.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchAtlas.c...
src/gpu/GrBatchAtlas.cpp:358: fAtlasGeneration++;
On 2015/03/26 17:06:55, bsalomon wrote:
> Is it worth detecting when this wraps and bumping again?

Maybe, your call.  I could also make it a uint64_t.  I think its really unlikely
to wrap, and the bug would just be a bit of corrupted text.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
File src/gpu/GrBatchFontCache.cpp (right):

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.cpp:35: static const int sAtlasIndices[] = {
On 2015/03/26 17:06:55, bsalomon wrote:
> If these are one to one why have two enums? I was expecting that
k8888_AtlasType
> might have a mix of color bitmap glyphs and lcd glyphs. But if that's not true
> then this seems unnecessary.\
> 
> nit: We usually prefix these "g" or "k". There is some internal variation on
> whether "k" means "kompile time konstant" and g means a "non-const global" or
> whether "k" means "integral konstant" and "g" means "any global that's
non-const
> or non-integral". I dunno that we need more variation :) 

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
File src/gpu/GrBatchFontCache.h (right):

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:24: *  glyphid->strike
On 2015/03/26 17:06:56, bsalomon wrote:
> I don't understand the gr or sk font code much at all but it occurs to me that
> this says this maps glyphids to strikes but it seems like the function that
> takes a glyph id returns a GrGlyph not a "strike".
> 
> Looking at GrGlyph it appears to refer to a rectangle in a plot and some
> metadata.
> 
> I'm not trying to be pedantic or at all blame this change for the confusion.
The
> reason I mention it is that I've always had a hard time understanding our font
> code and part of that is the sparse and not very descriptive comments and
jargon
> that doesn't seem intuitive to me. It's been like that forever and is probably
> better now than it used to be. I'd like us to make it better every time we
touch
> any of this code.
> 
> To that end:
> 
> Does this map glyphIds for a given strike to GrGlyphs?
> 
> Should GrGlyph be renamed? GrGlyphAtlasEntry? GrGlyphAtlasLocation? I'd rather
> make it a little more verbose if it adds clarity.

I added a comment.  I think we need to overhaul how all of these classes work
together in a follow on CL

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:75: class GrBatchFontCache {
On 2015/03/26 17:06:56, bsalomon wrote:
> /** This class manages a set GrBatchAtlases that store cached glyphs...  ...It
> uses GrGrBatchStrike for mumble mumble... distance field, bitmap (grayscale,
> color, lcd)... mumble */

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:94: void refGlyph(GrGlyph*,
GrBatchAtlas::BatchToken);
On 2015/03/26 17:06:56, bsalomon wrote:
> I think ref has a pretty specific meaning in Skia nomenclature. Can we rename
> this?
> 
> Haven't looked at the impl yet, but maybe something like
"glyphUsedWithToken()"?
> "recordGlyphToken()"? "setLastGlyphToken()"?
> 
> Also, it's kind of asking for documentation.

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:101: uint32_t atlasGeneration(GrMaskFormat) const;
On 2015/03/26 17:06:56, bsalomon wrote:
> doc?

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:122: enum AtlasType {
On 2015/03/26 17:06:56, bsalomon wrote:
> Maybe comment about how this relates to mask type? I assume it really refers
to
> the config of the backing texture but not the interpretation of the values
> (coverage, color).

I deleted this enum

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:133: static GrMaskFormat AtlasIndexToMaskFormat(int
atlasIndex);
On 2015/03/26 17:06:56, bsalomon wrote:
> Is this 1 to 1?

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:50: struct BitmapTextBlob : public SkRefCnt {
On 2015/03/26 17:06:56, bsalomon wrote:
> Comment on what this encapsulates? I get why it is what it is, but to a naive
> reader it looks like a mish mash of stuff.

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:53: struct TextInfo {
On 2015/03/26 17:06:56, bsalomon wrote:
> "PerFormatInfo"?
> 
> Comment somewhere that we consider an Info invalidated whenever a plot is
purged
> from the atlas without regard to whether any of these glyphs' plot was purged.

Acknowledged.

https://codereview.chromium.org/1011403004/diff/240001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:106: GrBatchTextStrike* fStrike;
On 2015/03/26 17:06:56, bsalomon wrote:
> Is this a current strike that is often updated? fCurrStrike?

I only use this to avoid getting the strike multiple times within a glyph, I can
TOTALLY clean this up and remove the state, but for now I renamed.

Jvsquare

A few more comments: https://codereview.chromium.org/1011403004/diff/260001/src/gpu/GrAtlas.cpp File src/gpu/GrAtlas.cpp (right): https://codereview.chromium.org/1011403004/diff/260001/src/gpu/GrAtlas.cpp#newcode228 src/gpu/GrAtlas.cpp:228: fTexture = fGpu->createTexture(desc, true, NULL, ...

5 years, 9 months ago (2015-03-27 15:07:30 UTC) #16

joshualitt

https://codereview.chromium.org/1011403004/diff/260001/src/gpu/GrAtlas.cpp File src/gpu/GrAtlas.cpp (right): https://codereview.chromium.org/1011403004/diff/260001/src/gpu/GrAtlas.cpp#newcode228 src/gpu/GrAtlas.cpp:228: fTexture = fGpu->createTexture(desc, true, NULL, 0); On 2015/03/27 15:07:29, ...

5 years, 8 months ago (2015-03-30 14:56:44 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011403004/360001

5 years, 8 months ago (2015-03-31 17:58:40 UTC) #22

commit-bot: I haz the power

Committed patchset #18 (id:360001) as https://skia.googlesource.com/skia/+/eed1dae04932483579b02c10f0706127d3f5d984

5 years, 8 months ago (2015-03-31 18:04:59 UTC) #23

joshualitt

A revert of this CL (patchset #18 id:360001) has been created in https://codereview.chromium.org/1050633002/ by joshualitt@google.com. ...

5 years, 8 months ago (2015-03-31 18:32:28 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011403004/400001

5 years, 8 months ago (2015-03-31 20:26:30 UTC) #27

commit-bot: I haz the power

5 years, 8 months ago (2015-03-31 20:32:09 UTC) #28

Message was sent while issue was closed.

Committed patchset #20 (id:400001) as
https://skia.googlesource.com/skia/+/7c3a2f834e0ba3f11a3129d5348b393efcc9b0e1

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Issue 1011403004: BitmapTextBatch and BitmapTextBlob (Closed)

Description

Patch Set 1 #

Patch Set 2 : working? #

Patch Set 3 : added flag #

Patch Set 4 : ready for review #

Patch Set 5 : tweak #

Patch Set 6 : feedback incorporated #

Patch Set 7 : tidy #

Patch Set 8 : typo #

Patch Set 9 : feedback inc #

Patch Set 10 : feedback inc #

Patch Set 11 : small fix #

Patch Set 12 : tidying #

Patch Set 13 : feedback inc #

Patch Set 14 : feedback inc #

Patch Set 15 : feedback inc #

Patch Set 16 : rebase #

Patch Set 17 : compiler warning #

Patch Set 18 : windows warning #

Patch Set 19 : fix gcc issue #

Patch Set 20 : fix for segfault #

Messages