Closed Bug 622482 Opened 9 years ago Closed 9 years ago

Support subpixel-AA when drawing to transparent surfaces with DirectWrite

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: roc, Assigned: bas.schouten)

References

(Depends on 1 open bug)

Details

(Whiteboard: [hardblocker][january 15])

Attachments

(9 files, 2 obsolete files)

22.48 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
36.34 KB, image/png
Details
1.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.88 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.12 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
60.95 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.68 KB, patch
Details | Diff | Splinter Review
3.31 KB, patch
Details | Diff | Splinter Review
63.21 KB, patch
Details | Diff | Splinter Review
We need this to get subpixel-AA in chrome when Aero Glass is enabled.
blocking2.0: --- → ?
Depends on: 612846
Blocks: 594325
Bas, IIRC, you were going to work on this? I might be wrong - it might have been Matt. Reassign as necessary...
Assignee: nobody → bas.schouten
blocking2.0: ? → betaN+
Whiteboard: [hard blocker]
Whiteboard: [hard blocker] → [hardblocker]
I did work on this, and I've even got a bunch of code done, and a working proof of concept. There's some concerns though that we need to discuss, probably monday on the GFx meeting.
Preparing for this work I'm moving the D2D glyph rasterization code. We've wanted to do this for a while and it will make this patch much easier.
Attachment #502565 - Flags: review?(jmuizelaar)
I think it's feasible to land this by January 15th if Jeff finds time to do the reviews.
Whiteboard: [hardblocker] → [hardblocker][january 15]
In the course of working on this bug I noticed a missing cairo_region_destroy call in _cairo_d2d_try_fastblit's early return path. I decided to add a helper class (I'll be adding another use-case for it soon) to prevent mistakes like this leak in the future.
Attachment #503331 - Flags: review?(jmuizelaar)
So with the patch queue I have for this bug, this is the rendering difference I see for the URL bar, where we previously had no subpixel-AA.
I suggest we start raking the benefits of the new code as soon as we land it, so we should allow subpixel AA on surfaces that don't need component alpha. Even though there's no component alpha code yet.
Attachment #503340 - Flags: review?(roc)
Attachment #503340 - Flags: review?(roc) → review+
Comment on attachment 502565 [details] [diff] [review]
Part 1: Move D2D Glyph rasterization code into cairo-d2d-surface

Looks very good provided it's just doing the move and there's nothing else interesting going on here.
Attachment #502565 - Flags: review?(jmuizelaar) → review+
Comment on attachment 503331 [details] [diff] [review]
Part 3: Add stack based helper for region destruction.

Overall I'd prefer if the code was simple enough to not require this thing, but I'd rather it be correct.

>+/* Stack-based helper to manage region destruction. */
>+struct RegionPtr
>+{
>+    RegionPtr() : mRegion(NULL)
>+    { }
>+    RegionPtr(cairo_region_t *region) : mRegion(region)
>+    { }
>+
>+    void Set(cairo_region_t *region) { mRegion = region; }
>+
>+    ~RegionPtr() { if (mRegion) cairo_region_destroy (mRegion); }
>+
>+    cairo_region_t *mRegion;
>+};
>+

Please use a more cairo like naming style since this isn't Direct2d related.
Attachment #503331 - Flags: review?(jmuizelaar) → review-
(In reply to comment #9)
> Comment on attachment 503331 [details] [diff] [review]
> Please use a more cairo like naming style since this isn't Direct2d related.

I'm fine with changing this but at the time I believe we agreed strict C++ non-cairo code would use our naming styles (like for example RefPtr) and C code related to cairo uses the cairo naming style. This is a little in the middle, it operates on a cairo object but it can never be part of cairo, since it's C++. Do you really want to change this even though it's a C++ object?
This factors out glyph run creation so we can re-use that code in the new manual code.
Attachment #503525 - Flags: review?(jmuizelaar)
(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 503331 [details] [diff] [review]
> > Please use a more cairo like naming style since this isn't Direct2d related.
> 
> I'm fine with changing this but at the time I believe we agreed strict C++
> non-cairo code would use our naming styles (like for example RefPtr) and C code
> related to cairo uses the cairo naming style. This is a little in the middle,
> it operates on a cairo object but it can never be part of cairo, since it's
> C++. Do you really want to change this even though it's a C++ object?

I looked back at the RefPtr bug and felt like the camel casing was justified because it operated on COM objects with "AddRef" and "Release" methods. This works on cairo objects so I think matching the cairo style makes more sense.
Attachment #503525 - Flags: review?(jmuizelaar) → review+
Attachment #503331 - Attachment is obsolete: true
Attachment #503549 - Flags: review?(jmuizelaar)
This adds a texture that can be used for most textruns to avoid texture creation having to occur.
Attachment #503550 - Flags: review?(jmuizelaar)
Comment on attachment 503549 [details] [diff] [review]
Part 3: Add stack based helper for region destruction. v2


>  * Helper functions.
>  */
> 
>+/* Stack-based helper to manage region destruction. */
>+struct region_ptr
>+{

I'd call it cairo_region_auto_ptr or something like that.

>+    region_ptr() : region(NULL)
>+    { }
>+    region_ptr(cairo_region_t *in_region) : region(in_region)
>+    { }
>+
>+    void Set(cairo_region_t *in_region) { region = in_region; }

Set is still capitalized.

>     }
> 
>     /* Region we need to clip this operation to */
>     cairo_region_t *clipping_region = NULL;
>     cairo_region_t *region;
>+    region_ptr regionPtr;

I'd prefer no capital P
Attachment #503549 - Flags: review?(jmuizelaar) → review+
Comment on attachment 503550 [details] [diff] [review]
Part 3.5: Create texture for cairo_d2d_device to be used for most textruns

Can we initialize this lazily?
(In reply to comment #17)
> Comment on attachment 503550 [details] [diff] [review]
> Part 3.5: Create texture for cairo_d2d_device to be used for most textruns
> 
> Can we initialize this lazily?

I don't think there's much of a point, this exists -per device- i.e. this is a one off texture creation on startup. I mean, don't get me wrong, sure we can, but any firefox user using Windows 7 will get one of these to draw the UI anyway.
Comment on attachment 503550 [details] [diff] [review]
Part 3.5: Create texture for cairo_d2d_device to be used for most textruns

Remember to addjust vmram usage
Attachment #503550 - Flags: review?(jmuizelaar) → review+
Comment on attachment 503563 [details] [diff] [review]
Part 4: Manually draw glyphs to our surface when we know we can but it's transparent.


This method is pretty long. If there are any good candidates for moving stuff out into different functions that would be nice.

>+cairo_int_status_t
>+_cairo_dwrite_manual_show_glyphs_on_d2d_surface(void			    *surface,
>+						cairo_operator_t	     op,
>+						const cairo_solid_pattern_t *source,
>+						cairo_glyph_t		    *glyphs,
>+						int			     num_glyphs,
>+						cairo_dwrite_scaled_font_t  *scaled_font,
>+						cairo_clip_t		    *clip)
>+{
>+    cairo_dwrite_font_face_t *dwriteff = reinterpret_cast<cairo_dwrite_font_face_t*>(scaled_font->base.font_face);
>+    cairo_d2d_surface_t *dst = reinterpret_cast<cairo_d2d_surface_t*>(surface);
>+
>+    UINT16 *indices = new UINT16[num_glyphs];
>+    DWRITE_GLYPH_OFFSET *offsets = new DWRITE_GLYPH_OFFSET[num_glyphs];
>+    FLOAT *advances = new FLOAT[num_glyphs];
>+    BOOL transform = FALSE;
>+    HRESULT hr;
>+
>+    cairo_region_t *clip_region = NULL;

Add a comment about why complex clips are not supported

>+    if (clip) {
>+	_cairo_clip_get_region(clip, &clip_region);
>+	if (!clip_region) {
>+	    return CAIRO_INT_STATUS_UNSUPPORTED;
>+	}
>+    }
>+
>+    _cairo_d2d_set_clip(dst, NULL);
>+    dst->rt->Flush();
>+
>+    DWRITE_GLYPH_RUN run;
>+    _cairo_dwrite_glyph_run_from_glyphs(glyphs, num_glyphs, scaled_font, &run, &transform);
>+
>+    RefPtr<IDWriteGlyphRunAnalysis> analysis;

Why declare analysis here?

>+    DWRITE_MATRIX dwmat = _cairo_dwrite_matrix_from_matrix(&scaled_font->mat);
>+
>+    RefPtr<IDWriteRenderingParams> params;
>+    dst->rt->GetTextRenderingParams(&params);

I'd add an empty line here

>+    DWRITE_RENDERING_MODE renderMode;

and take this one out

>+    dwriteff->dwriteface->GetRecommendedRenderingMode((FLOAT)scaled_font->base.font_matrix.yy,
>+						      1.0f,
>+						      DWRITE_MEASURING_MODE_NATURAL,
>+						      params,
>+						      &renderMode);
>+
>+    // Deal with rendering modes CreateGlyphRunAnalysis doesn't accept.
>+    if (renderMode == DWRITE_RENDERING_MODE_DEFAULT) {
>+	// As per DWRITE_RENDERING_MODE documentation, pick Natural for font
>+	// sizes under 16 ppem
>+	renderMode = scaled_font->base.font_matrix.yy < 16.0f ?
>+		     DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL :
>+                     DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC;

I think this might be easy to read as a regular 'if'

>+    } else if (renderMode == DWRITE_RENDERING_MODE_OUTLINE) {
>+	renderMode = DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC;
>+    }
>+
>+    DWriteFactory::Instance()->CreateGlyphRunAnalysis(&run,
>+						      1.0f,
>+						      transform ? &dwmat : 0,
>+						      renderMode,
>+						      DWRITE_MEASURING_MODE_NATURAL,
>+						      0,
>+						      0,
>+						      &analysis);

might as well align all of the arguments (this applies elsewhere too)

>+
>+    delete [] run.glyphIndices;
>+    delete [] run.glyphAdvances;
>+    delete [] run.glyphOffsets;
>+
>+    RECT bounds;
>+    analysis->GetAlphaTextureBounds(DWRITE_TEXTURE_CLEARTYPE_3x1,
>+				    &bounds);

Align &bounds with DWRITE_TEXTU....

>+
>+    cairo_rectangle_int_t cairo_bounds;
>+
>+    cairo_bounds.x = bounds.left;
>+    cairo_bounds.y = bounds.top;
>+    cairo_bounds.width = (bounds.right - bounds.left);
>+    cairo_bounds.height = (bounds.bottom - bounds.top);

Probably worth adding a helper that converts from RECT to cairo_rectangle_int_t.

>+
>+    cairo_region_t *region;
>+    if (clip) {
>+	region = cairo_region_copy(clip_region);
>+
>+	cairo_region_intersect_rectangle(region, &cairo_bounds);
>+    } else {
>+	region = cairo_region_create_rectangle(&cairo_bounds);
>+    }
>+
>+    region_ptr regionPtr(region);

uncapitalize regionPtr

>+
>+    if (cairo_region_is_empty(region)) {
>+	// Nothing to do.
>+	return CAIRO_INT_STATUS_SUCCESS;
>+    }
>+ 
>+    int bufferSize = cairo_bounds.width * cairo_bounds.height * 3;
>+
>+    if (!bufferSize) {
>+	// Width == 0 || height == 0

remove capital W

>+	return CAIRO_INT_STATUS_SUCCESS;
>+    }
>+
>+    int alignedBufferSize = cairo_bounds.width * cairo_bounds.height * 4;

move alignedBufferSize closer to where it's used.

>+
>+    BYTE *texture = new BYTE[bufferSize + 1];

explain what the +1 is for

>+    analysis->CreateAlphaTexture(DWRITE_TEXTURE_CLEARTYPE_3x1, &bounds, texture, bufferSize);
>+
>+    RefPtr<ID3D10ShaderResourceView> srView;
>+    ID3D10Device1 *device = dst->device->mD3D10Device;
>+
>+    int textureWidth, textureHeight;
>+
>+    if (cairo_bounds.width < TEXT_TEXTURE_WIDTH &&
>+	cairo_bounds.height < TEXT_TEXTURE_HEIGHT)

align cairo_bounds

>+    {
>+	D3D10_MAPPED_TEXTURE2D texMap;
>+	HRESULT hr = dst->device->mTextTexture->Map(0, D3D10_MAP_WRITE_DISCARD, 0, &texMap);
>+
>+	if (FAILED(hr)) {
>+	    delete [] texture;
>+	    return CAIRO_INT_STATUS_UNSUPPORTED;
>+	}
>+
>+	BYTE *alignedTextureData = (BYTE*)texMap.pData;
>+	for (int y = 0; y < cairo_bounds.height; y++) {
>+	    for (int x = 0; x < cairo_bounds.width; x++) {
>+		*((int*)(alignedTextureData + (y * texMap.RowPitch) + x * 4)) =
>+		    *((int*)(texture + (y * cairo_bounds.width + x) * 3));
>+	    }
>+	}

Add a comment about what you're doing here and make it more readable if you can.

>+
>+	dst->device->mTextTexture->Unmap(0);
>+
>+	srView = dst->device->mTextTextureView;
>+
>+	textureWidth = TEXT_TEXTURE_WIDTH;
>+	textureHeight = TEXT_TEXTURE_HEIGHT;
>+    } else {
>+	BYTE *alignedTextureData = new BYTE[alignedBufferSize];
>+	for (int y = 0; y < cairo_bounds.height; y++) {
>+	    for (int x = 0; x < cairo_bounds.width; x++) {
>+		*((int*)(alignedTextureData + (y * cairo_bounds.width + x) * 4)) =
>+		    *((int*)(texture + (y * cairo_bounds.width + x) * 3));
>+	    }
>+	}

likewise.

>+
>+	D3D10_SUBRESOURCE_DATA data;
>+  
>+	CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM,
>+				   cairo_bounds.width, cairo_bounds.height,
>+				   1, 1);
>+	desc.Usage = D3D10_USAGE_IMMUTABLE;
>+
>+	data.SysMemPitch = cairo_bounds.width * 4;
>+	data.pSysMem = alignedTextureData;
>+
>+	RefPtr<ID3D10Texture2D> tex;
>+	device->CreateTexture2D(&desc, &data, &tex);
>+
>+	delete [] alignedTextureData;
>+
>+	device->CreateShaderResourceView(tex, NULL, &srView);

The return value of CreateTexture... and CreateShader... should be checked? Make sure you check anything else you want to handle the failure of.

>+
>+	textureWidth = cairo_bounds.width;
>+	textureHeight = cairo_bounds.height;
>+    }
>+    delete [] texture;
>+
>+    RefPtr<ID3D10Texture2D> dstTexture;
>+
>+    dst->surface->QueryInterface(&dstTexture);
>+
>+    if (!dst->buffer_rt_view) {
>+	hr = device->CreateRenderTargetView(dstTexture, NULL, &dst->buffer_rt_view);
>+	if (FAILED(hr)) {
>+	    return CAIRO_INT_STATUS_UNSUPPORTED;

When do you expect this to fail? and is UNSUPPORTED a good return value in the case of failure?

>+	}
>+    }
>+

[snip]

>+
>+    float colorVal[] = { float(source->color.red * source->color.alpha),
>+	                 float(source->color.green * source->color.alpha),
>+			 float(source->color.blue * source->color.alpha),
>+			 float(source->color.alpha) };

Aligning all of the * source->color.alpha will look cleaner.

>+    textColor->SetFloatVector(colorVal);
>+
>+    float quadDescVal[4];
>+    float texCoordsVal[4];
>+
>+    for (int i = 0; i < cairo_region_num_rectangles(region); i++) {
>+	cairo_rectangle_int_t quad;
>+	cairo_region_get_rectangle(region, i, &quad);
>+
>+	quadDescVal[0] = -1.0f + ((float)quad.x / (float)tDesc.Width) * 2.0f;
>+	quadDescVal[1] = 1.0f - ((float)quad.y / (float)tDesc.Height) * 2.0f;
>+	quadDescVal[2] = ((float)quad.width / (float)tDesc.Width) * 2.0f;
>+	quadDescVal[3] = -((float)quad.height / (float)tDesc.Height) * 2.0f;
>+	
>+	texCoordsVal[0] = (float)(quad.x - cairo_bounds.x) / textureWidth;
>+	texCoordsVal[1] = (float)(quad.y - cairo_bounds.y) / textureHeight;
>+	texCoordsVal[2] = (float)((quad.x + quad.width) - cairo_bounds.x) / textureWidth;
>+	texCoordsVal[3] = (float)((quad.y + quad.height) - cairo_bounds.y) / textureHeight;

The above is a little messy. No suggestions on how to fix it though.

>+
>+	quadDesc->SetFloatVector(quadDescVal);
>+	texCoords->SetFloatVector(texCoordsVal);
>+
>+	_cairo_d2d_setup_for_blend(dst->device);
>+	technique->GetPassByIndex(0)->Apply(0);
>+
>+	ID3D10ShaderResourceView *srViewPtr = srView;
>+	device->PSSetShaderResources(0, 1, &srViewPtr);
>+
>+	device->Draw(4, 0);
>+    }
>+
>+    return CAIRO_INT_STATUS_SUCCESS;
>+}
>+

[snip]

>     }
> 
>+    if (op == CAIRO_OPERATOR_OVER && source->type == CAIRO_PATTERN_TYPE_SOLID &&
>+	dst->base.content != CAIRO_CONTENT_COLOR &&
>+	dst->base.permit_subpixel_antialiasing &&
>+	dwritesf->antialias_mode == CAIRO_ANTIALIAS_SUBPIXEL) {
>+	const cairo_solid_pattern_t *solid_src =
>+	    reinterpret_cast<const cairo_solid_pattern_t*>(source);

Please add some documentation about why drawing the glyphs manually is necessary.

>+	status = _cairo_dwrite_manual_show_glyphs_on_d2d_surface(surface,
>+								 op,
>+								 solid_src,
>+								 glyphs,
>+								 num_glyphs,
>+								 dwritesf,
>+								 clip);

Overall, I have no objections to the approach but it could use some cleanup. I'm giving an r+ because I'd rather this get in sooner than add the additional latency of another round of review, but it's probably worth rerequesting review on the version that gets committed and I'll do a quick post-commit review.
Attachment #503563 - Flags: review?(jmuizelaar) → review+
Adjusted per review comments.
Attachment #503549 - Attachment is obsolete: true
Attachment #504139 - Flags: review?(jmuizelaar)
Comment on attachment 504139 [details] [diff] [review]
Part 3: Add stack based helper for region destruction. v3

This was already r+'ed with these changes. This is just the checkin version of the patch.
Attachment #504139 - Flags: review?(jmuizelaar)
Attachment #504140 - Attachment is patch: true
Attachment #504140 - Attachment mime type: application/octet-stream → text/plain
Checkin version for post-commit review as requested.
Attachment #504141 - Attachment is patch: true
Attachment #504141 - Attachment mime type: application/octet-stream → text/plain
Target Milestone: --- → mozilla2.0b10
This seems to be causing Bug 626227  on my system.
I don't use panorama so i can't comment for that but this seems to slowdown the whole gui alot on my system an example is having a bookmark sidebar open and scroll it , with today nightly it is very very slow with 50% cpu usage

8800GT D2D/Direct3D 10
Hrm, I will investigate the reported perf regressions. Our Talos test suites don't seem to have been affected.
I have a relatively slow machine where most serious GUI performance regressions are immediately obvious and I DO NOT see any slowdown. Testing on Win 7 on a three year old 1.4GHz Core 2 Duo.
I can only reproduce this with D3D10 layers switching to D3D9 fixes it
(In reply to comment #29)
> I have a relatively slow machine where most serious GUI performance regressions
> are immediately obvious and I DO NOT see any slowdown. Testing on Win 7 on a
> three year old 1.4GHz Core 2 Duo.

Is that with D3D9 or D3D10? ATI, Intel or Nvidia?
(In reply to comment #31) 
> Is that with D3D9 or D3D10? ATI, Intel or Nvidia?

No performance problems with Integrated Intel graphics.

Direct2D Enabled: true
DirectWrite Enabled: true (6.1.7600.20830)
WebGL Renderer: Intel -- Mobile Intel(R) 4 Series Express Chipset Family -- 2.1.0 - Build 8.15.10.2202
GPU Accelerated Windows: 1/1 Direct3D 10
Hrm, when I get the perf problems on my NVidia machine we seem to be getting a significant amount of page faults and the kernel setting up DMA-able memory, this is interesting and means my idea of using staging textures with CopySubResource might actually work.
(In reply to comment #34)
> regression.
> 
> http://forums.mozillazine.org/viewtopic.php?p=10316669#p10316669

Would you be so kind to file a bug on this? I'm probably going to need some help from the people with more knowledge of fonts than myself to figure this one out.
(In reply to comment #35)
> (In reply to comment #34)
> > regression.
> > 
> > http://forums.mozillazine.org/viewtopic.php?p=10316669#p10316669
> 
> Would you be so kind to file a bug on this? I'm probably going to need some
> help from the people with more knowledge of fonts than myself to figure this
> one out.

filed.

bug 626299
Blocks: 625029
i found your try-server build here http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bschouten@mozilla.com-3fab9e3c17f3/try-w32/

and that fixes the very slow scrolling problem with history/bookmark sidebar for me
Blocks: 626581
No longer blocks: 626581
Depends on: 626581
Night and Day. Bas, you are a hero. Mozillian of the month (if that exists).
Depends on: 700088
Depends on: 1095954
Depends on: 1095954
You need to log in before you can comment on or make changes to this bug.