Investigate Direct2D Rendering Backend

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

(Depends on: 6 bugs, Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 34 obsolete attachments)

671.62 KB, image/png
Details
926.46 KB, image/png
Details
34.97 KB, image/png
Details
10.84 KB, patch
jimm
: review+
Details | Diff | Splinter Review
141.61 KB, image/png
Details
3.84 KB, patch
ted
: review+
Details | Diff | Splinter Review
258.18 KB, image/png
Details
145.63 KB, patch
Details | Diff | Splinter Review
8.99 KB, patch
Details | Diff | Splinter Review
4.39 KB, text/plain
Details
135 bytes, text/html
Details
557 bytes, text/html
Details
46.11 KB, image/png
Details
831 bytes, patch
Details | Diff | Splinter Review
103.16 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.03 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Direct2D is the 'new way' to go for rendering on updated Windows Vista and Windows 7 platforms. It's worthwhile investigating the possibility of creating a Cairo backend for it.
(Assignee)

Comment 1

9 years ago
Created attachment 413243 [details] [diff] [review]
Cairo DirectWrite and D2D backends

Attaching diffs for making this work.
(Assignee)

Comment 2

9 years ago
Created attachment 413244 [details] [diff] [review]
mozilla changes for direct2d and directwrite support

These changes go into the tree. Note some things in here are somewhat hacky. I assume win32 surfaces work and gfxWindowsSurface still generates those when asking for something else than an hwnd surface. This means only uxtheme still gets to use Win32 surfaces. Also USE_WIN_SURFACE in libpr0n is disabled. Which might slightly affect GDI performance.
(Assignee)

Updated

9 years ago
Attachment #413244 - Attachment is patch: true
Attachment #413244 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 3

9 years ago
Created attachment 413245 [details] [diff] [review]
Cairo DirectWrite and D2D backends

This time patch with all the files in it.
Attachment #413243 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 413246 [details] [diff] [review]
mozilla changes for direct2d and directwrite support v1

And also complete set of mozilla tree changes
Attachment #413244 - Attachment is obsolete: true
Duplicate of this bug: 529643
OS: Windows NT → Windows 7
(Assignee)

Comment 6

9 years ago
Created attachment 414316 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v2

I've added the current DWrite and D2D backends, would be good to get some eyes on that already.

Note the implementation is not cairo-correct. It doesn't implement all possible cairo-operators. It does function correctly for practically all Firefox uses (that I've seen so far) though.
Attachment #413245 - Attachment is obsolete: true
Attachment #414316 - Flags: review?(tellrob)
Attachment #414316 - Flags: review?(jmuizelaar)

Comment 7

9 years ago
Created attachment 414333 [details]
Screenshot of a d2d build with 106 dpi

I tried the d2d build from http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some issues when the dpi setting is not the default (of 96).

The screenshot compares the d2d build on the left and a "normal" build on the right. My system is configured to 106 dpi (110% font size).
It looks like Windows tries to zoom the interface itself (as it would when the application is not flagged as DPI aware). Note that the clickable regions are not scaled (the hit area for a button doesn't map to the drawn region of that button for instance).

I'm wondering if using a manifest instead of calling SetProcessDPIAware() would help here. From http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
"Note - SetProcessDPIAware is subject to a possible race condition if a DLL caches dpi settings during initialization. For this reason, it is recommended that dpi-aware be set through the application (.exe) manifest rather than by calling SetProcessDPIAware."
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> Created an attachment (id=414333) [details]
> Screenshot of a d2d build with 106 dpi
> I tried the d2d build from
> http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some
> issues when the dpi setting is not the default (of 96).
> The screenshot compares the d2d build on the left and a "normal" build on the
> right. My system is configured to 106 dpi (110% font size).
> It looks like Windows tries to zoom the interface itself (as it would when the
> application is not flagged as DPI aware). Note that the clickable regions are
> not scaled (the hit area for a button doesn't map to the drawn region of that
> button for instance).
> I'm wondering if using a manifest instead of calling SetProcessDPIAware() would
> help here. From
> http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
> "Note - SetProcessDPIAware is subject to a possible race condition if a DLL
> caches dpi settings during initialization. For this reason, it is recommended
> that dpi-aware be set through the application (.exe) manifest rather than by
> calling SetProcessDPIAware."

I suspect part of this issue is something with uxtheme and how I blend it. Another part is probably simply the fact that I'm no obeying D2D DPI considerations everywhere yet. I'll ook into this, thanks forthe screenshot. It could be that the theme library atleast needs to do something wit SetProcessDPIAware, it could also very well be a blending issue of the theme.
(Assignee)

Comment 9

9 years ago
(In reply to comment #7)
> Created an attachment (id=414333) [details]
> Screenshot of a d2d build with 106 dpi
> 
> I tried the d2d build from
> http://www.bassified.nl/firefox-3.7a1pre.en-US.win32.d2d.zip and noticed some
> issues when the dpi setting is not the default (of 96).
> 
> The screenshot compares the d2d build on the left and a "normal" build on the
> right. My system is configured to 106 dpi (110% font size).
> It looks like Windows tries to zoom the interface itself (as it would when the
> application is not flagged as DPI aware). Note that the clickable regions are
> not scaled (the hit area for a button doesn't map to the drawn region of that
> button for instance).
> 
> I'm wondering if using a manifest instead of calling SetProcessDPIAware() would
> help here. From
> http://msdn.microsoft.com/en-us/library/ms633543%28VS.85%29.aspx:
> "Note - SetProcessDPIAware is subject to a possible race condition if a DLL
> caches dpi settings during initialization. For this reason, it is recommended
> that dpi-aware be set through the application (.exe) manifest rather than by
> calling SetProcessDPIAware."
I believe I've fixed your issue (well, I've adjusted the build to give the same result as GDI, this is different from other browsers though, which actually render sites bigger). Please let me know if it worked! I uploaded the package to the same URL as before.

Comment 10

9 years ago
Yes, it works. Well done :-)
Here's the start of some comments:

1. A couple of functions have inconsistent casting like the following:

+    if (((cairo_scaled_font_t*)scaled_font)->backend->type ==
+	CAIRO_FONT_TYPE_DW) {
+	    return CAIRO_INT_STATUS_UNSUPPORTED;
+    }
+    cairo_dw_scaled_font_t *dwsf = static_cast<cairo_dw_scaled_font_t*>(scaled_font);

2. compute_mask_alpha and invert_mask_alpha in cairo-dw-font.c look like copies from cairo-win32-font.c. Note: this code seems like it has bugs too (see bug 363861)

3. GeometryRecorder could use a comment saying that it's for getting glyph_paths and the class definition could be moved closer to the only user.

4. cairo_d2d_surface_create_with_size() should just be called cairo_d2d_surface_create()

5. _cairo_d2d_show_glyphs can return an uninitialized status

6. All of the (cairo_int_status_t)CAIRO_STATUS_SUCCESS are pretty painful to look at. Perhaps we could add a CAIRO_STATUS_INT_SUCCESS macro to the backend.

7. A bunch of the backend functions do:
    if (((cairo_surface_t*)surface)->type != CAIRO_SURFACE_TYPE_D2D) {
	return (cairo_status_t)CAIRO_INT_STATUS_UNSUPPORTED;
    }
  which shouldn't be needed.
More comments:

8. cairo_dw_show_glyphs_on_d2d_surface() should probably be in cairo-d2d-surface.c unless having it near to the other dw_show_glyphs is of value.

9. What's the performance difference between _dw_draw_glyphs_to_gdi_surface_d2d
and dw_draw_glyphs_to_gdi_surface_gdi. Under what conditions do we expect _dw_draw_glyphs_to_gdi_surface_d2d fail but _dw_draw_glyphs_to_gdi_surface_gdi to succeed?
10. instead of using _cairo_fixed_to_double() in _d2d_point_from_cairo_point we should add a _cairo_fixed_to_float() method and use that.

11. The line wrapping seems a little weird in _d2d_snapshot_detached(), cairo doesn't have a hard limit on line length and unwrapping the call to cairo_surface_get_user_data() should make things more readable.

12. It might also make sense to change the type of existingBitmap to cached_bitmap * and cast the return value of cairo_surface_get_user_data().

In fact how about:

    cached_bitmap *existingBitmap = (cached_bitmap*)cairo_surface_get_user_data (
                                                             surface,
                                                             &bitmap_key);
    
    if (existingBitmap) {
	    exisitingBitmap->dirty = true;
    }

13. Related to the above, the cairo style when wrapping function calls is:

a_function_with_a_really_really_long_name(param1,
                                          param2,
                                          param3);
14. RadialGradientBrushProperties() could use _cairo_fixed_to_float too

15. No need to wrap:
if (surfacePatter->surface->type == CAIRO_SURFACE_TYPE_D2D)
and similar.

16.

+	    if (surfacePattern->surface->type ==
+		CAIRO_SURFACE_TYPE_IMAGE) {
+		cairo_matrix_t translate;
+		memset(&translate, 0, sizeof(cairo_matrix_t));
+		cairo_matrix_t origmat = mat;
+		translate.xx = 1.0;
+		translate.yy = 1.0;
+		translate.x0 = -1.0;
+		translate.y0 = -1.0;
+		cairo_matrix_multiply(&mat, &translate, &mat);
+	    }

looks like it could just be cairo_matrix_translate (&mat, -1, -1);

17.

+		    /**
+		     * Trick here, we create a temporary rectangular
+		     * surface with 1 pixel margin on each side. This
+		     * provides a rectangular transparent border, that
+		     * will ensure CLAMP acts as EXTEND_NONE.
+		     */
+		    unsigned int tmpWidth = srcSurf->width + 2;
+		    unsigned int tmpHeight = srcSurf->height + 2;
+		    unsigned char *tmp = new unsigned char[tmpWidth * tmpHeight * 4];
+		    memset(tmp, 0, tmpWidth * tmpHeight * 4);
+		    for (int y = 0; y < srcSurf->height; y++) {
+			memcpy(
+			    tmp + tmpWidth * 4 * y + tmpWidth * 4 + 4, 
+			    srcSurf->data + y * srcSurf->stride, 
+			    srcSurf->width * 4);
+		    }
+

I think it might be worth moving this to a utility function. We could optimize this by avoiding memsetting the whole array, but I don't care that much about the performance of EXTEND_NONE so I comment about the potential optimization is probably enough.


18.

+	 * Nice axist aligned rectangular clip, try and do our best to keep it
+	 * that way.

'axist' has a typo
I'm wondering, how often do we hit _cairo_d2d_acquire_dest_image and _cairo_d2d_acquire_source_image during normal browsing and under what circumstances do we hit it?
Also, how do these backends do on our reftests?
If you click on the spinning box on this page (http://www.zachstronaut.com/posts/2009/08/07/jquery-animate-css-rotate-scale.html) a couple times. The text seems to switch between large and small once per rotation.
19. I think we should be able to use IDWriteGlyphRunAnalysis in _cairo_dw_scaled_show_glyphs instead of trying to manually extract the mask.
Assignee: nobody → bas.schouten
(Assignee)

Comment 19

9 years ago
So, first of all, thanks for the really quick review:
Here's a quick reply to all your points, some still have questions, once those are answered I'll upload a new patch.
 
1. Done
2. No longer needed, see point 19.
3. Done
4. Done
5. Done
6. Done
7. Done

8. It just seemed to make sense there, to have all that code in one centralised
place, so that if anyone fixes a bug there, they won't forget to fix it in the other
show_glyphs.

9. Depends wildly on the machine. Due to some comments on the DWrite bug I've now
switched to always using GDI for the GDI surfaces, and only trying D2D on GDI
when there's a special #define set.

10. Done
11. Done
12. Done

13. Eww... that means you get really long lines when having long arguments.. oh well. Want me to change -all- of those?

14. Done
15. Done
16. Done

17. Comment done.. you really think this will be more readable when put in a seperate
function? It wouldn't be used anywhere else...

18. Done
19. Done.
(In reply to comment #19)
> 
> 8. It just seemed to make sense there, to have all that code in one centralised
> place, so that if anyone fixes a bug there, they won't forget to fix it in the
> other
> show_glyphs.

Fair enough.

> 13. Eww... that means you get really long lines when having long arguments.. oh
> well. Want me to change -all- of those?

It would be nice :) I find the style where arguments only wrap to an additional indent level harder to read. Most of the time it shouldn't make the lines that much longer.

> 17. Comment done.. you really think this will be more readable when put in a
> seperate
> function? It wouldn't be used anywhere else...

Maybe, I don't feel that strongly about it though.
(Assignee)

Comment 21

9 years ago
Created attachment 415024 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v3

Patch with jrmuizel's review comments processed.
Attachment #414316 - Attachment is obsolete: true
Attachment #415024 - Flags: review?(tellrob)
Attachment #415024 - Flags: review?(jmuizelaar)
Attachment #414316 - Flags: review?(tellrob)
Attachment #414316 - Flags: review?(jmuizelaar)
Can you upload a tryserver build?
Thanks!
(Assignee)

Comment 23

9 years ago
(In reply to comment #22)
> Can you upload a tryserver build?
> Thanks!

I cannot, since the try servers do not have the DirectX SDK installed yet. This is work in progress :). I did make a build myself though, which can be gotten from my blog (http://www.basschouten.com/)
This hunk could use a comment explaining what's going on. i.e. the format conversion taking place. Aligning the equals symbols will also make it easier to read.

+	for (unsigned int y = 0; y < height; y++) {
+	    for (unsigned int x = 0; x < width; x++) {
+		mask_surface->data[y * mask_surface->stride + x * 4] = surface[y * width * 3 + x * 3 + 2];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 1] = surface[y * width * 3 + x * 3 + 1];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 2] = surface[y * width * 3 + x * 3];
+		mask_surface->data[y * mask_surface->stride + x * 4 + 3] = surface[y * width * 3 + x * 3 + 2];
+	    }
+	}
1. It doesn't look we respect the font antialiasing options that have been set.
Depends on: 532106
No longer depends on: 532106
(In reply to comment #19)
> 13. Eww... that means you get really long lines when having long arguments.. oh
> well. Want me to change -all- of those?

You got close but I see a bunch of lines that are indented like:

long_function_name(param_a,
                  param_b,
                  param_c);

instead of:

long_function_name(param_a,
                   param_b,
                   param_c);

as it should be.
2. 
+/**
+ * Get D2D point from cairo point
+ *
+ * \param point Cairo point
+ * \return D2D point
+ */
+D2D1_POINT_2F
+_d2d_point_from_cairo_point(const cairo_point_t *point)
+{
+    return D2D1::Point2F(_cairo_fixed_to_float(point->x),
+			 _cairo_fixed_to_float(point->y));
+}
+
+/**
+ * Get D2D color from cairo color
+ *
+ * \param color Cairo color
+ * \return D2D color
+ */
+D2D1_COLOR_F
+_cairo_d2d_color_from_cairo_color(const cairo_color_t &color)
+{
+    return D2D1::ColorF((FLOAT)color.red, 
+			(FLOAT)color.green, 
+			(FLOAT)color.blue,
+			(FLOAT)color.alpha);
+}

The comments for these functions don't really add any value. I'd suggest removing them.

3.
+/**
+ * Via a little trick this is just used to determine when a surface has been
+ * modified.
+ */
+void _d2d_snapshot_detached(cairo_surface_t *surface)
+{
+    cached_bitmap *existingBitmap = (cached_bitmap*)cairo_surface_get_user_data(surface, &bitmap_key);
+    if (existingBitmap) {
+	    cached_bitmap *cachebitmap = (cached_bitmap*)existingBitmap;
+	    cachebitmap->dirty = true;
+    }
+}

No need for cached_bitmap to exist like I suggested previously.

4. _d2d_release_bitmap is probably a better name than _d2d_clear_bitmap. There are also three different 'bitmap's in that function, if we can come up with different names for them that would be better.

5. _cairo_d2d_create_brush_for_pattern feels really dense to me. It would be nice if we could add some line breaks to break it into more paragraphy pieces so it's easier to read. Here are some suggestions:

after this hunk:

+		    memset(tmp, 0, tmpWidth * tmpHeight * Bpp);
+		    for (int y = 0; y < srcSurf->height; y++) {
+			memcpy(
+			    tmp + tmpWidth * Bpp * y + tmpWidth * Bpp + Bpp, 
+			    srcSurf->data + y * srcSurf->stride, 
+			    srcSurf->width * Bpp);
+		    }

after this hunk:

+	    DXGI_FORMAT format;
+	    unsigned int Bpp;
+	    if (srcSurf->format == CAIRO_FORMAT_ARGB32) {
+		format = DXGI_FORMAT_B8G8R8A8_UNORM;
+		Bpp = 4;
+	    } else if (srcSurf->format == CAIRO_FORMAT_RGB24) {
+		format = DXGI_FORMAT_B8G8R8A8_UNORM;
+		Bpp = 4;
+	    } else if (srcSurf->format == CAIRO_FORMAT_A8) {
+		format = DXGI_FORMAT_A8_UNORM;
+		Bpp = 1;
+	    } else {
+		return NULL;
+	    }

etc.

This hunk has weird wrapping:
+	    cached_bitmap *cachebitmap = 
+		(cached_bitmap*)cairo_surface_get_user_data(
+		surfacePattern->surface,
+		&bitmap_key);

This line has an extra space:

+	    D2D1_MATRIX_3X2_F matrix =  _cairo_d2d_matrix_from_matrix(&mat);
6. cairo_dwrite_show_glyphs_on_d2d_surface is still public.

7. What happens when you pass a NULL brush to the D2D functions? We do this in a bunch of places, when we should perhaps be returning CAIRO_STATUS_INT_UNSUPPORTED.

8. Why does _cairo_d2d_present_backbuffer take a void * instead of a cairo_surface_t *?

9. I'd suggest putting line breaks after each section that compute a stroke property in _cairo_d2d_create_strokestyle_for_stroke_style.

10. The error handling in _cairo_d2d_create_similar might be better in the goto style like _cairo_win32_scaled_font_type1_text_to_glyphs(). However, if you do go this way I'd suggest more descriptive names than FAIL[0-3].

11. _cairo_d2d_acquire_dest_image doesn't seem to deal with surfaces of types other than CAIRO_CONTENT_COLOR_ALPHA. I believe it should.

12. This hunk should use MIN() instead of open-coding it.

+	    /** We have a clip rect, intersect of two rects is simple */
+	    D2D1_RECT_F newRect;
+	    newRect.top = d2dsurf->clipRect->top < _cairo_fixed_to_float(box.p1.y) ?
+		_cairo_fixed_to_float(box.p1.y) : d2dsurf->clipRect->top;
+	    newRect.left = d2dsurf->clipRect->left < _cairo_fixed_to_float(box.p1.x) ?
+		_cairo_fixed_to_float(box.p1.x) : d2dsurf->clipRect->left;
+	    newRect.bottom = d2dsurf->clipRect->bottom < _cairo_fixed_to_float(box.p2.y) ?
+		d2dsurf->clipRect->bottom : _cairo_fixed_to_float(box.p2.y);
+	    newRect.right = d2dsurf->clipRect->right < _cairo_fixed_to_float(box.p2.x) ?
+		d2dsurf->clipRect->right : _cairo_fixed_to_float(box.p2.x);
+	    *d2dsurf->clipRect = newRect;
+	    return CAIRO_INT_STATUS_SUCCESS;

We should be able to do without newRect too and just assign directly to clipRect too right?

13. Can _cairo_dwrite_scaled_font_init_glyph_metrics fail? If not it should return void instead of cairo_status_t.
(In reply to comment #23)
> I cannot, since the try servers do not have the DirectX SDK installed yet. This
> is work in progress :). I did make a build myself though, which can be gotten
> from my blog (http://www.basschouten.com/)

You should file a bug in mozilla.org : Release Engineering on this, and make it blocking this bug. If the Try Servers don't have this SDK, then I will bet that the build machines don't have it either, so you won't be able to land this without that happening.
(Assignee)

Updated

9 years ago
Depends on: 529938
(Assignee)

Comment 30

9 years ago
Re: Last review stuff.

2. I agree, but since they were helper functions and not implementations of existing, documented functions it seemed to make sense for them to have documentation. To have them searchable and stuff if you'd build documentations from the comment.
3. Doh, I changed the type of existing bitmap, but not the rest, my bad!
7. I can only see me forgetting to null check the brush in Paint (which is a bug obviously :)), did I miss anything? D2D will definitely crash in !debug mode.
11. Good catch, surprised that didn't give any problems.
12. Erm.. yeah, what I did is silly. We should do that.
13. It seemed consistent with how the Win32 backend did it. It probably -should- be able to fail and properly error check.
Depends on: 532106
(In reply to comment #30)
> Re: Last review stuff.
> 
> 2. I agree, but since they were helper functions and not implementations of
> existing, documented functions it seemed to make sense for them to have
> documentation. To have them searchable and stuff if you'd build documentations
> from the comment.

Does anyone build this documentation? I've never seen it and I don't really know who would use it.

> 3. Doh, I changed the type of existing bitmap, but not the rest, my bad!
> 7. I can only see me forgetting to null check the brush in Paint (which is a
> bug obviously :)), did I miss anything? D2D will definitely crash in !debug
> mode.

cairo_dwrite_show_glyphs_on_d2d_surface also doesn't check.
(Assignee)

Comment 32

9 years ago
Created attachment 415771 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v4

A new patch with jrmuizel's comments processed (hopefully correctly this time!) Also some changes in line with some changes made based on the review comments(i.e. create_similar actually supports its content types), and some bug fixes.

The FAIL labels should probably be used in the other two create surface functions as well, but I'm still thinking on how to do that exactly. Done for create_similar though.
Attachment #415024 - Attachment is obsolete: true
Attachment #415024 - Flags: review?(tellrob)
Attachment #415024 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 years ago
Attachment #415771 - Flags: review?(tellrob)
Attachment #415771 - Flags: review?(jmuizelaar)

Updated

9 years ago
Blocks: 363861

Comment 33

9 years ago
Bas, very snappy builds and I haven't found really any problems except one.  When searching on google, the right third or so of the page content flashes grey.

STR:
1) GO to http://www.google.com/search?hl=en&q=adblock&btnG=Search&aq=f&oq=&aqi=
2) Hit the search button or refresh the page
(Assignee)

Updated

9 years ago
Depends on: 534787
(Assignee)

Comment 34

9 years ago
Created attachment 417612 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v5

Latest version of my D2D backend code. Featuring horrible loops to facilitate tiling on large surfaces and other such fun! Also lazily creates a lot more for performance reasons, disabled V-sync, and some other general fixes.
Attachment #415771 - Attachment is obsolete: true
Attachment #415771 - Flags: review?(tellrob)
Attachment #415771 - Flags: review?(jmuizelaar)
1. Why do you use D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS?
2. All of the internal methods should be static.
3. I think we need a better name for bufferTexture. I don't think buffer is the best name for what it does. Also _cairo_d2d_buffer_texture() is a little confusing because it reads like we are buffering a texture instead of getting the buffer texture.

4. I don't really understand this comment:
 * Ensure that the surface has an up to date surface bitmap. Used for
 * window surfaces which cannot have a surface bitmap directle related
 * to their backbuffer for some reason.
What does it mean for window surfaces to have a surface bitmap directly relate to their backbuffer?

Some nits: directle is typo'ed and "up to date" can be hyphenated.

5. Why do we use DXGI_FORMAT_UNKNOWN in _cairo_d2d_update_surface_bitmap()? Does the format not matter?

6. cairo_d2d_scroll takes a "void *" surface when it should take a cairo_surface_t*.

7. _cairo_d2d_present_backbuffer shouldn't have a hyphen if it's a public function and both _cairo_d2d_present_backbuffer and cairo_d2d_scroll should both do surface type testing like cairo_xlib_surface_get_drawable()
(Assignee)

Comment 37

9 years ago
1. A CG researcher e-mailed me after looking at the patches that Direct2D uses that internally when you let D2D create the device so that I should try it. It gave a small performance boost on my machine, so I decided to keep it :). Doesn't mean I shouldn't look into why it helps though :)

4. You cannot create a bitmap around a backbuffer surface for reasons I do not completely understand (it will fail with an E_INVALIDARG). Meaning they need a special texture to store their graphical data which is wrapped by a D2D bitmap if a window surface is ever used in a surface pattern. All other D2D surfaces use a texture as their backing store so can have a bitmap directly.

5. It will then automatically use the format of the texture, preventing us having to do a conditional there.

Other than that - gotya.
(In reply to comment #37)
> 1. A CG researcher e-mailed me after looking at the patches that Direct2D uses
> that internally when you let D2D create the device so that I should try it. It
> gave a small performance boost on my machine, so I decided to keep it :).
> Doesn't mean I shouldn't look into why it helps though :)

The msdn documentation for D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS suggests against using it, so we should put as much info into a comment as we know.

> 4. You cannot create a bitmap around a backbuffer surface for reasons I do not
> completely understand (it will fail with an E_INVALIDARG). Meaning they need a
> special texture to store their graphical data which is wrapped by a D2D bitmap
> if a window surface is ever used in a surface pattern. All other D2D surfaces
> use a texture as their backing store so can have a bitmap directly.

This explanation looks like it would be a better comment than the one we currently have :)

> 
> 5. It will then automatically use the format of the texture, preventing us
> having to do a conditional there.

It's worth adding a comment to this effect there.

Comment 39

9 years ago
I would be that "CG researcher " ;). I can understand how the msdn documentation on D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS can be misleading. In fact, that flag gives no such indication. I pointed this out to Bas in my email. However, Microsoft is in fact using this flag to indicate "light weight" DX applications. By light weight they are essentially referring to applications that are not games. The idea is that when you create a DX game, the driver assumes that you will pretty much have a single instance and therefore it doesn't try to hold back when it comes to GPU resource allocation as long as it can crank out performance. In other words, the priority in regular DX applications is to make that one application run as fast as you can. For "light weight" applications, including D2D applications, the priorities are a bit different. Now you are no longer going to have a single (or very few) instances. You can have a lot of them (say, for example, a separate DX context/device per browser tab). In such cases, the GPU resource allocation scheme changes.

Since D2D is implemented on top of DX10, Microsoft uses this particular flag to indicate to the graphics driver that a "light weight" application is being created. I know that this is a bit confusing when a user looks up the documentation on MSDN, but thats just how it is at the moment. Perhaps in the future Microsoft will create a separate flag to indicate this more explicitly. But until then, there is no reason why the optimizations should not be made use of.
(In reply to comment #39)
> I would be that "CG researcher " ;). I can understand how the msdn
[snip]
> of.

This looks like a good comment to add :) Thanks.
Bas, can you post a patch of the mozilla integration so I try and find out what's going on in bug 535396.
8. _cairo_d2d_paint() assumes that CAIRO_OPERATOR_CLEAR is done with a solid pattern. CLEAR shouldn't use the source pattern at all.

9. Have you tried running the cairo test suite?
(Assignee)

Comment 43

9 years ago
Created attachment 418095 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v1
(Assignee)

Comment 44

9 years ago
Created attachment 418096 [details] [diff] [review]
Build System DirectX/DWrite/Direct2D mods
(Assignee)

Comment 45

9 years ago
Created attachment 418097 [details] [diff] [review]
Widget Direct2D Code v1
Attachment #413246 - Attachment is obsolete: true
(Assignee)

Comment 46

9 years ago
Note that with the above patches, it should work. They might not be commented very well yet though, I still need to address that, especially the DirectWrite code. Although that code in this form is only temporary (until jfkthame can land his stuff), it's probably still good to document that a lot better.

You also need to make sure USE_WIN_SURFACE is undefined in modules/libpr0n, and ideally we'll want a fix for 534787. Although that will only show on some image loading/animated gifs.
(Assignee)

Updated

9 years ago
Depends on: 535590
(Assignee)

Comment 47

9 years ago
Created attachment 418208 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v2

Corrected for latest repos head.
Attachment #418095 - Attachment is obsolete: true
(Assignee)

Comment 48

9 years ago
Created attachment 418210 [details] [diff] [review]
Widget Direct2D Code v2
Attachment #418097 - Attachment is obsolete: true
(Assignee)

Comment 49

9 years ago
Created attachment 418211 [details] [diff] [review]
Build System DirectX/DWrite/Direct2D mods v2
Attachment #418096 - Attachment is obsolete: true
A quick comment on the the thebes patch: we should get rid of the Microsoft sample  code if possible.

Updated

9 years ago
Attachment #418211 - Attachment is patch: true
Attachment #418211 - Attachment mime type: application/octet-stream → text/plain

Comment 51

9 years ago
Looks like Cairo DirectWrite and Direct2D Backends need to be updated as well. First, there was a build error because cairo_d2d_present_backbuffer() is actually declared as _cairo_d2d_present_backbuffer(). I corrected that, and the build was successful. However, the executable crashes on launch due to a NULL pointer access  in function: _cairo_d2d_release_dest_image() at the following line:

d2dsurf->surfaceBitmap->CopyFromMemory(&rect, data.pData, data.RowPitch);

Here, d2dsurf->surfaceBitmap is NULL.
(Assignee)

Comment 52

9 years ago
(In reply to comment #51)
> Looks like Cairo DirectWrite and Direct2D Backends need to be updated as well.
> First, there was a build error because cairo_d2d_present_backbuffer() is
> actually declared as _cairo_d2d_present_backbuffer(). I corrected that, and the
> build was successful. However, the executable crashes on launch due to a NULL
> pointer access  in function: _cairo_d2d_release_dest_image() at the following
> line:
> 
> d2dsurf->surfaceBitmap->CopyFromMemory(&rect, data.pData, data.RowPitch);
> 
> Here, d2dsurf->surfaceBitmap is NULL.

That's not unlikely actually, that function -should- never be called. I know what the bug is. Forgot to update it because I'd never experience it. It'd be interesting to know why it moves to the software fallback functions.

Comment 53

9 years ago
looking at the code, the fallback functions seem to be called unconditionally. The logic seems to be:

if (backend_func_available)
{
    status = call_backend_func();
    if (status == UNSUPPORTED) return error;
}

status = call_fallback_func();

So unless the backend function returns unsupported, the fallback function will always be called along with it. I see this sort of logic all across the call stack.
Comment on attachment 418208 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v2

>+// xxx - used in FontEntry.  should be trimmed, moz code doesn't use
>+//       exceptions.  use gfxSparseBitSet instead?
>+#include <bitset>

Less STL is a good thing for our codebase. Maybe there's something under XPCOM to handle this?

>diff --git a/gfx/thebes/src/gfxD2DSurface.cpp b/gfx/thebes/src/gfxD2DSurface.cpp
>new file mode 100644
>--- /dev/null
>+++ b/gfx/thebes/src/gfxD2DSurface.cpp
>@@ -0,0 +1,48 @@
>+#include "gfxD2DSurface.h"
>+#include "cairo.h"
>+#include "cairo-win32.h"
>+
>+gfxD2DSurface::gfxD2DSurface(HWND aWnd)
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    Init(cairo_d2d_surface_create_for_hwnd(aWnd));
>+#endif
>+}
>+
>+gfxD2DSurface::gfxD2DSurface(cairo_surface_t *csurf)
>+{
>+    Init(csurf);
>+}
>+
>+gfxD2DSurface::gfxD2DSurface(const gfxIntSize& size,
>+                             gfxImageFormat imageFormat)
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    Init(cairo_d2d_surface_create((cairo_format_t)imageFormat, size.width, size.height));
>+#endif
>+}
>+
>+gfxD2DSurface::~gfxD2DSurface()
>+{
>+}
>+
>+void
>+gfxD2DSurface::Present()
>+{
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    cairo_d2d_present_backbuffer(CairoSurface());
>+#endif
>+}
>+
>+void
>+gfxD2DSurface::Scroll(const nsIntPoint &aDelta, const nsIntRect &aClip)
>+{
>+    cairo_rectangle_t rect;
>+    rect.x = aClip.x;
>+    rect.y = aClip.y;
>+    rect.width = aClip.width;
>+    rect.height = aClip.height;
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    cairo_d2d_scroll(CairoSurface(), aDelta.x, aDelta.y, &rect);
>+#endif
>+}

What is the point of the #define? Would it be simpler to not define gfxD2DSurface in this case? At the least this could be collapsed to a single #ifdef block.

>+// -- From Microsoft Samples --
>+
>+// The following macros define the minimum required platform.  The minimum required platform
>+// is the earliest version of Windows, Internet Explorer etc. that has the necessary features to run 
>+// your application.  The macros work by enabling all features available on platform versions up to and 
>+// including the version specified.
>+
>+// Modify the following defines if you have to target a platform prior to the ones specified below.
>+// Refer to MSDN for the latest info on corresponding values for different platforms.
>+
>+#ifndef WINVER                  // Minimum platform is Windows 7
>+#define WINVER 0x0601
>+#endif
>+
>+#ifndef _WIN32_WINNT            // Minimum platform is Windows 7
>+#define _WIN32_WINNT 0x0601
>+#endif
>+
>+#ifndef _WIN32_WINDOWS          // Minimum platform is Windows 7
>+#define _WIN32_WINDOWS 0x0601
>+#endif
>+
>+#ifndef WIN32_LEAN_AND_MEAN     // Exclude rarely-used stuff from Windows headers
>+#define WIN32_LEAN_AND_MEAN
>+#endif
>+
>+#ifndef NOMINMAX                // Use the standard's templated min/max
>+#define NOMINMAX
>+#endif
>+
>+#ifndef _USE_MATH_DEFINES       // want PI defined
>+#define _USE_MATH_DEFINES
>+#endif

I am skeptical that we need do these defines to get things to work. Overriding these values in a header is extremely dangerous. I don't have a link handy, but these represent the minimum supported version of any file that includes this header.

>+
>+////////////////////////////////////////
>+// Common headers:
>+
>+// C headers:
>+#include <stdlib.h>
>+#include <malloc.h>
>+#include <memory.h>
>+#include <math.h>
>+
>+// C++ headers:
>+#include <algorithm>
>+#include <numeric>
>+#include <string>
>+#include <vector>
>+#include <utility>
>+#include <limits>

None of these should be necessary.

>+// Windows headers:
>+
>+#include <windows.h>
>+#include <windowsx.h>
>+#include <unknwn.h>

I would be very surprised if we need these two headers.
>+
>+#include <dwrite.h>
>+#include <intsafe.h>
>+#include <strsafe.h>

And these last two.

>+
>+////////////////////////////////////////
>+// Common macro definitions:
>+
>+#ifndef HINST_THISCOMPONENT
>+EXTERN_C IMAGE_DOS_HEADER __ImageBase;
>+#define HINST_THISCOMPONENT ((HINSTANCE)&__ImageBase)
>+#endif
>+
>+// Use the double macro technique to stringize the actual value of s
>+#define STRINGIZE_(s) STRINGIZE2_(s)
>+#define STRINGIZE2_(s) #s
>+
>+#define FAILURE_LOCATION L"\r\nFunction: " TEXT(__FUNCTION__) L"\r\nLine: " TEXT(STRINGIZE_(__LINE__))

None of these macros are used in this patch.

>+#if (_MSC_VER >= 1200) // want to use std::min and std::max
>+#undef min
>+#undef max
>+#define min(x,y) _cpp_min(x,y)
>+#define max(x,y) _cpp_max(x,y)
>+#endif

I believe we have our own versions of these and this patch doesn't seem to use them. Redefining these in a header is poor form.

>+
>+// Ignore unreferenced parameters, since they are very common
>+// when implementing callbacks.
>+#pragma warning(disable : 4100)

DANGER.

>+////////////////////////////////////////
>+// COM inheritance helpers.
>+
>+
>+// Releases a COM object and nullifies pointer.
>+template <typename InterfaceType>
>+inline void SafeRelease(InterfaceType** currentObject)
>+{
>+    if (*currentObject != NULL)
>+    {
>+        (*currentObject)->Release();
>+        *currentObject = NULL;
>+    }
>+}
>+
>+
>+// Acquires an additional reference, if non-null.
>+template <typename InterfaceType>
>+inline InterfaceType* SafeAcquire(InterfaceType* newObject)
>+{
>+    if (newObject != NULL)
>+        newObject->AddRef();
>+
>+    return newObject;
>+}
>+
>+
>+// Sets a new COM object, releasing the old one.
>+template <typename InterfaceType>
>+inline void SafeSet(InterfaceType** currentObject, InterfaceType* newObject)
>+{
>+    SafeAcquire(newObject);
>+    SafeRelease(currentObject);
>+    *currentObject = newObject;
>+}
>+

None of these are used.

>+
>+// Maps exceptions to equivalent HRESULTs,
>+inline HRESULT ExceptionToHResult() throw()
>+{
>+    try
>+    {
>+        throw;  // Rethrow previous exception.
>+    }
>+    catch(std::bad_alloc&)
>+    {
>+        return E_OUTOFMEMORY;
>+    }
>+    catch (...)
>+    {
>+        return E_FAIL;
>+    }
>+}
>+
>+
>+// Generic COM base implementation for classes, since DirectWrite uses
>+// callbacks for several different kinds of objects, particularly the
>+// script analysis source/sink.
>+//
>+// Example:
>+//
>+//  class TextAnalysis : public ComBase<QiList<IDWriteTextAnalysisSink> >
>+//
>+template <typename InterfaceChain>
>+class ComBase : public InterfaceChain
>+{
>+public:
>+    explicit ComBase() throw()
>+    :   refValue_(0)
>+    { }
>+
>+    // IUnknown interface
>+    IFACEMETHOD(QueryInterface)(IID const& iid, OUT void** ppObject)
>+    {
>+        *ppObject = NULL;
>+        InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+        if (*ppObject == NULL)
>+            return E_NOINTERFACE;
>+
>+        AddRef();
>+        return S_OK;
>+    }
>+
>+    IFACEMETHOD_(ULONG, AddRef)()
>+    {
>+        return InterlockedIncrement(&refValue_);
>+    }
>+
>+    IFACEMETHOD_(ULONG, Release)()
>+    {
>+        ULONG newCount = InterlockedDecrement(&refValue_);
>+        if (newCount == 0)
>+            delete this;
>+
>+        return newCount;
>+    }

I don't think these need to be interlocked.

>+
>+    virtual ~ComBase()
>+    { }
>+
>+protected:
>+    ULONG refValue_;

This should be mRefValue.

>+
>+private:
>+    // No copy construction allowed.
>+    ComBase(const ComBase& b);
>+    ComBase& operator=(ComBase const&);
>+};
>+
>+
>+struct QiListNil
>+{
>+};
>+
>+
>+// When the QueryInterface list refers to itself as class,
>+// which hasn't fully been defined yet.
>+template <typename InterfaceName, typename InterfaceChain>
>+class QiListSelf : public InterfaceChain
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+
>+// When this interface is implemented and more follow.
>+template <typename InterfaceName, typename InterfaceChain = QiListNil>
>+class QiList : public InterfaceName, public InterfaceChain
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return InterfaceChain::QueryInterfaceInternal(iid, ppObject);
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+
>+// When the this is the last implemented interface in the list.
>+template <typename InterfaceName>
>+class QiList<InterfaceName, QiListNil> : public InterfaceName
>+{
>+public:
>+    inline void QueryInterfaceInternal(IID const& iid, OUT void** ppObject) throw()
>+    {
>+        if (iid != __uuidof(InterfaceName))
>+            return;
>+
>+        *ppObject = static_cast<InterfaceName*>(this);
>+    }
>+};
>+
>+// -- End of MS sample code --

This QiList machinery is neat and clever but is it really necessary for the implementation?

>+// Singleton accessor for DirectWriteFactory. Threadsafe. This
>+// will obviously leak the factory at shutdown.

You could observe the xpcom-shutdown event to safely shut this down I think.

>+// DirectWrite is not available on all platforms.
>+typedef HRESULT (WINAPI*DWriteCreateFactoryFunc)(
>+  __in   DWRITE_FACTORY_TYPE factoryType,
>+  __in   REFIID iid,
>+  __out  IUnknown **factory
>+);

Please move this type declaration into the DWriteFactory class.

>+class DWriteFactory
>+{
>+public:
>+    static IDWriteFactory *Instance()
>+    {
>+        if (!PR_AtomicSet(&mInitialized, 1)) {
>+            nsresult rv;
>+            PRBool forceGDI = PR_FALSE;
>+            nsCOMPtr<nsIPrefService> prefSvc = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+            if (NS_SUCCEEDED(rv)) {
>+		        nsCOMPtr<nsIPrefBranch> prefBranch;
>+		        rv = prefSvc->GetBranch("font.engine.", getter_AddRefs(prefBranch));
>+		        if (NS_SUCCEEDED(rv)) {

Nit: remove the tabs here.

>+                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
>+                    if (NS_FAILED(rv)) {
>+                        forceGDI = PR_FALSE;
>+                    }
>+                }
>+            }
>+#ifndef CAIRO_HAS_DWRITE_FONT
>+            // If this is the case we need to make sure we use GDI.
>+            forceGDI = PR_TRUE;
>+#endif

You could refactor this to not check the pref if we're not compiled with DWrite

>+            mInitialized = PR_TRUE;
>+	        // If we force GDI we become initialized but will never give a factory.

Nit: tab

>+            if (!forceGDI) {
>+                DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
>+                    GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
>+
>+                if (createDWriteFactory) {
>+                    HRESULT hr = createDWriteFactory(
>+                        DWRITE_FACTORY_TYPE_SHARED,
>+                        __uuidof(IDWriteFactory),
>+                        reinterpret_cast<IUnknown**>(&mInstance));
>+                }
>+            }

This need not be compiled in if DWRITE support isn't enabled. This may in fact cause a perf hit.

>+        }
>+	    return mInstance;

Nit: tab

>+HRESULT STDMETHODCALLTYPE
>+gfxDWriteFontFileLoader::CreateStreamFromKey(const void *fontFileReferenceKey, 
>+					     UINT32 fontFileReferenceKeySize, 
>+					     IDWriteFontFileStream **fontFileStream)

Ok, it's probably easier to just let you remove all the tabs and not point them out each time.

>+IFACEMETHODIMP TextAnalysis::GetTextBeforePosition(
>+    UINT32 textPosition,
>+    OUT WCHAR const** textString,
>+    OUT UINT32* textLength
>+    ) throw()

Are these throw()s absolutely necessary? I think it carries quite a performance hit.

>+gfxWindowsFontEntry *
>+gfxWindowsDWriteFontFamily::FindFontEntry(const gfxFontStyle &aFontStyle)
>+{
>+    PRBool needsBold;
>+    return static_cast<gfxWindowsDWriteFontEntry*> (FindFontForStyle(aFontStyle, needsBold));

Hmm...intentional? The following is unreachable code.

>+    IDWriteFont *font;
>+
>+    DWRITE_FONT_STRETCH stretch;
>+    if (!aFontStyle.stretch) {
>+        stretch = DWRITE_FONT_STRETCH_NORMAL;
>+    } else {
>+        // TODO: Remove 'magic number' 4 and do proper code.
>+        stretch = static_cast<DWRITE_FONT_STRETCH>(aFontStyle.stretch - 4);
>+    }
>+
>+    DWRITE_FONT_STYLE style;
>+    switch(aFontStyle.style) {
>+        case NS_FONT_STYLE_NORMAL:
>+            style = DWRITE_FONT_STYLE_NORMAL;
>+            break;
>+        case NS_FONT_STYLE_ITALIC:
>+            style = DWRITE_FONT_STYLE_ITALIC;
>+            break;
>+        case NS_FONT_STYLE_OBLIQUE:
>+            style = DWRITE_FONT_STYLE_OBLIQUE;
>+            break;
>+    }
>+    

Nit: trailing spaces

>+PRBool
>+gfxWindowsDWriteFontEntry::MatchesGenericFamily(const nsACString &aGeneric) const
>+{
>+    // TODO: Implement me.
>+    return PR_TRUE;
>+}

Still a TODO?

>+PRBool
>+gfxWindowsDWriteFontEntry::SupportsRange(PRUint8 aRangeBit)
>+{
>+    // TODO: Implement me!!
>+    return PR_TRUE;
>+}

Ditto.

>+    DWRITE_FONT_STRETCH stretch;
>+    if (!fontGroup->mStyle.stretch) {
>+        stretch = DWRITE_FONT_STRETCH_NORMAL;
>+    } else {
>+        // TODO: Remove 'magic number' 4 and do proper code.
>+        stretch = static_cast<DWRITE_FONT_STRETCH>(fontGroup->mStyle.stretch - 4);

What is this code doing? The -4 must go.

>+
>+gfxWindowsDWriteFontGroup::~gfxWindowsDWriteFontGroup()
>+{
>+    mFontCollection->Release();
>+}

Can't we use nsRefPtr/nsCOMPtr for this instead of manually releasing?

>+void 
>+gfxWindowsDWriteFontGroup::UpdateFontList()
>+{
>+    // if user font set is set, check to see if font list needs updating
>+    if (mUserFontSet && mCurrGeneration != GetGeneration()) {
>+        // xxx - can probably improve this to detect when all fonts were found, so no need to update list
>+        mFonts.Clear();
>+        mFontEntries.Clear();
>+        InitFontList();
>+        mCurrGeneration = GetGeneration();

Hmm, I'm not sure what this code is supposed to do but the "xxx" is of course worrying.

>+    /**
>+     * There's an internal 16-bit limit on some things inside the analyzer.
>+     * to be on the safe side here we split up any ranges over 25000 characters.
>+     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
>+     */
>+    for(int i = 0; i < ranges.Length(); i++) {
>+	if (ranges[i].Length() > 25000) {
>+	    ranges.InsertElementAt(i + 1, gfxTextRange(ranges[i].start + 25000, ranges[i].end));
>+	    ranges[i + 1].font = ranges[i].font;
>+	    ranges[i].end = ranges[i].start + 25000;
>+	}

Can you #define 25000 as a named constant somewhere?

>diff --git a/gfx/thebes/src/gfxWindowsGDIFonts.cpp b/gfx/thebes/src/gfxWindowsGDIFonts.cpp
>+//#define FORCE_UNISCRIBE 1

Why is this commented out?

>+#define FORCE_PR_LOG

Debugging leftovers?

>+        IDWriteFontFamily *family;
>+        for (UINT32 i = 0; i < systemFonts->GetFontFamilyCount(); i++) {
>+            systemFonts->GetFontFamily(i, &family);
>+            IDWriteLocalizedStrings *familyNames;
>+            family->GetFamilyNames(&familyNames);
>+            WCHAR tbuf[256];
>+            familyNames->GetString(0, tbuf, 256);

Can we lose the magic numbers in favor of named constants? What if the familyName is too long? (does this case come up? seems unlikely).

>+    if (DWriteFactory::Instance()) {
>+        // TODO: Need to do more intelligent matching like down with normal win32 fonts.
>+        const PRUint32 ch = data->mCh;

What does this mean?

> gfxWindowsSurface::gfxWindowsSurface(const gfxIntSize& size, gfxImageFormat imageFormat) :
>     mOwnsDC(PR_FALSE), mForPrinting(PR_FALSE), mWnd(nsnull)
> {
>     if (!CheckSurfaceSize(size))
>         return;
>-
>     cairo_surface_t *surf = cairo_win32_surface_create_with_dib((cairo_format_t)imageFormat,
>-                                                                size.width, size.height);
>+								size.width, size.height);
>     Init(surf);
> 
>     if (CairoStatus() == 0)
>-        mDC = cairo_win32_surface_get_dc(CairoSurface());
>+	mDC = cairo_win32_surface_get_dc(CairoSurface());
>     else
>-        mDC = nsnull;
>+	mDC = nsnull;
> }
> 
> gfxWindowsSurface::gfxWindowsSurface(HDC dc, const gfxIntSize& size, gfxImageFormat imageFormat) :
>     mOwnsDC(PR_FALSE), mForPrinting(PR_FALSE), mWnd(nsnull)
> {
>     if (!CheckSurfaceSize(size))
>         return;
> 
>@@ -130,18 +130,19 @@ gfxWindowsSurface::GetImageSurface()
>         NS_WARNING ("GetImageSurface on an invalid (null) surface; who's calling this without checking for surface errors?");
>         return nsnull;
>     }
> 
>     NS_ASSERTION(CairoSurface() != nsnull, "CairoSurface() shouldn't be nsnull when mSurfaceValid is TRUE!");
> 
>     if (mForPrinting)
>         return nsnull;
>+    cairo_surface_t *isurf;
>+    isurf = cairo_win32_surface_get_image(CairoSurface());
> 
>-    cairo_surface_t *isurf = cairo_win32_surface_get_image(CairoSurface());
>     if (!isurf)
>         return nsnull;
> 
>     nsRefPtr<gfxASurface> asurf = gfxASurface::Wrap(isurf);
>     gfxImageSurface *imgsurf = (gfxImageSurface*) asurf.get();
>     NS_ADDREF(imgsurf);
>     return imgsurf;
> }

Some unnecessary changes here. Lose the tabs.

Comment 55

9 years ago
Bas, I think the DirectWrite and Direct2D changes should be kept separate, since we might take each on different schedules (e.g. take DirectWrite but wait on Direct2D).  Keep the DirectWrite code on a patch in 517642 and note the dependency between the code here.  

Also, the DirectWrite code is dependent on 493280 and 502906.
(In reply to comment #54)
> I am skeptical that we need do these defines to get things to work. Overriding
> these values in a header is extremely dangerous. I don't have a link handy, but
> these represent the minimum supported version of any file that includes this
> header.

Also note that we set some of these in configure:
http://mxr.mozilla.org/mozilla-central/source/configure.in#734

based on the value passed to --with-windows-version.
(Assignee)

Comment 57

9 years ago
(In reply to comment #55)
> Bas, I think the DirectWrite and Direct2D changes should be kept separate,
> since we might take each on different schedules (e.g. take DirectWrite but wait
> on Direct2D).  Keep the DirectWrite code on a patch in 517642 and note the
> dependency between the code here.  
> 
> Also, the DirectWrite code is dependent on 493280 and 502906.

Hrm, we could, I suppose do that. Not sure what the rest thinks. I could tear them apart, but we'd need to decide on the order. I could never make two patches which can both lad on the current repository head.

@Ted/Rob: Those common defines are all going as I replace the MS sample code used there.
My build crashes for me too.
(Assignee)

Comment 59

9 years ago
Created attachment 418433 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v6
Attachment #417612 - Attachment is obsolete: true
v6 works for me, but doesn't draw a bunch of images.
Depends on: 535396

Comment 61

9 years ago
OK, that fixes the crash issue. I am able to build and run the executable. But the rendering seems to be a bit off. I'm attaching a screenshot. The browser's navigation controls are not being blitted properly (they appear and disappear on mouse over), and the webpage content rendering is also out of place.

Comment 62

9 years ago
Created attachment 418445 [details]
rendering corruption with D2D backend v6
(Assignee)

Comment 63

9 years ago
Created attachment 418639 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v7

This fixes the crash on fallback, and a memory leak. Do note it's very important we find out why it's falling back in your builds, since it will have a huge performance impact, and really shouldn't happen. This is most likely a fill operation, we need to understand why cairo_d2d_fill fails.
Attachment #418433 - Attachment is obsolete: true
(Assignee)

Comment 64

9 years ago
Created attachment 418640 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v3

Thebes code with no more MS sample code. And part of Rob's comments processed. Those issues that I copy-pasted from WindowsGDIFonts I didn't fix yet.
Attachment #418208 - Attachment is obsolete: true

Comment 65

9 years ago
Bas, I have debugged this a bit and it looks like the reason why the fallback is kicking in is due to the failure of the D2D brush creation. In particular, the brush creation is failing when the type is CAIRO_PATTERN_TYPE_SURFACE, and the surface type is CAIRO_SURFACE_TYPE_WIN32. You don't have a case handler for that type. You have:

if ( type == CAIRO_SURFACE_TYPE_D2D)
{
...
}
else if ( type = CAIRO_SURFACE_TYPE_IMAGE )
{
...
}
else
{
return NULL;
}

so you're ending up returning NULL here and the fallback kicks in.

Comment 66

9 years ago
typo above:
else if ( type = CAIRO_SURFACE_TYPE_IMAGE )

is

else if ( type == CAIRO_SURFACE_TYPE_IMAGE )
(Assignee)

Comment 67

9 years ago
(In reply to comment #66)
> typo above:
> else if ( type = CAIRO_SURFACE_TYPE_IMAGE )
> 
> is
> 
> else if ( type == CAIRO_SURFACE_TYPE_IMAGE )

Doh! Ofcourse, Joe's patches for Image Frames. My appologies for not being more clear about this. You need my hack from bug 534787 until that is resolved! With that applied you should be all good. See my comment 46.

Comment 68

9 years ago
yep, that did the trick. everything is now functional in my build.
1. cairo_dwrite_show_glyphs_on_surface still has a cairo_public attribute. It shouldn't and should maybe start with an underscore... I can't remember the precedent here.
Some comments on the thebes patch:

1. All of the ifdef CAIRO_HAS_D2D_SURFACE in gfxD2DSurface.cpp
 should probably go away. Do we have any use for gfxD2DSurface.cpp when we don't have CAIRO_HAS_D2D_SURFACE?

2. gfxWindowsDWriteFont::ComputeMetrics() is a wall of code add some line breaks please.

3. Do you change gfxWindowsGDIFont other than renaming it? In fact, can we keep it as gfxWindowsFont for now, and change it's name in a letter patch. i.e. we'll have gfxWindowsFont and gfxWindowsDWriteFont instead of gfxWindowsGDIFont and gfxWindowsDWriteFont. That will cut down on the amount of noise and make reviewing easier.
(Assignee)

Comment 71

9 years ago
1a. Yeah, fair enough, missed that, sorry.
1b. Well, as previously discussed, if the class ceases to exist in that case it would mean ifdefs in gfxWindowsPlatform and nsWindowGfx.cpp and another file in Widget. Whereas now the ifdefs only exist in gfxD2DSurface.cpp, I don't mind either way.
3. gfxWindowsFont is now an abstract base class that also contains some non pure virtual functions implemented there that were previously in gfxWindowsGDIFonts. So unless we make that gfxWindowsFontCommon(which is less pretty I think) that won't work. We'd also still have changes to gfxWindowsFonts for the functions that are removed.

Comment 72

9 years ago
Created attachment 420043 [details]
screenshot, missing browser buttons

I'm seeing funky rendering, similar to that noted attachment 418445 [details].

Also, a bunch of downloadable font tests fail with the when building trunk with the four attached patches.

http://people.mozilla.org/~jdaggett/webfonts/chunkfive-test.html (font doesn't load)
http://people.mozilla.org/~jdaggett/webfonts/droidserif-test.html (some fonts don't load, rendering incorrect)
http://people.mozilla.org/~jdaggett/webfonts/gentium-test.html (fonts don't load)
http://people.mozilla.org/~jdaggett/webfonts/goudy1911-test.html
http://people.mozilla.org/~jdaggett/webfonts/historicaltext.html (images don't render)
http://people.mozilla.org/~jdaggett/tests/titilliumtest.html (fonts don't load)

Comment 73

9 years ago
Test config:

Dell XPS 9000 Core i7 
NVIDIA GeForce GTS 240
Windows 7 ja

Comment 74

9 years ago
One thing to note here, it looks like these patches require the Aug 2009 DX SDK to build:

http://www.microsoft.com/downloads/details.aspx?FamilyID=B66E14B8-8505-4B17-BF80-EDB2DF5ABAD4&displaylang=en
(Assignee)

Comment 75

9 years ago
(In reply to comment #72)
> Created an attachment (id=420043) [details]
> screenshot, missing browser buttons
> 
> I'm seeing funky rendering, similar to that noted attachment 418445 [details].

The solution to that problem, is listed above, it has to do with the USE_WIN_SURFACE define in modules/libpr0n.

Comment 76

9 years ago
(In reply to comment #75)
> The solution to that problem, is listed above, it has to do with the
> USE_WIN_SURFACE define in modules/libpr0n.

So you mean USE_WIN_SURFACE needs to be commented out in modules/libpr0n/src/imgFrame.cpp?  Or that this is simply a "known" issue?  Do these patches rely on a fix for a bug that is not in the depends list?
(Assignee)

Updated

9 years ago
Depends on: 534788
(Assignee)

Comment 77

9 years ago
(In reply to comment #76)
> (In reply to comment #75)
> > The solution to that problem, is listed above, it has to do with the
> > USE_WIN_SURFACE define in modules/libpr0n.
> 
> So you mean USE_WIN_SURFACE needs to be commented out in
> modules/libpr0n/src/imgFrame.cpp?  Or that this is simply a "known" issue?  Do
> these patches rely on a fix for a bug that is not in the depends list?
Yep, that's what I mean. See comment 46. I updated the depends list as well.

Comment 78

9 years ago
The reason many pages with downloadable fonts render incorrectly is that you don't initialize the new gfxWindowsDWriteFontEntry with the weight, style, and stretch properties from the proxy entry:

+    BOOL isSupported;
+    DWRITE_FONT_FILE_TYPE fileType;
+    UINT32 numFaces;
+
+    gfxWindowsDWriteFontEntry *entry = new gfxWindowsDWriteFontEntry(uniqueName, NULL, fontFile);
+
+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
+    if (!isSupported) {
+        delete entry;
+        return nsnull;
+    }
+    entry->mIsUserFont = PR_TRUE;

Instead you need to initialize the gfxWindowsDWriteFontEntry similar to the way the font entry is initialized in gfxWindowsGDIFontEntry::LoadFont:

    PRUint16 w = (aProxyEntry.mWeight == 0 ? 400 : aProxyEntry.mWeight);

    gfxWindowsGDIFontEntry *fe = gfxWindowsGDIFontEntry::CreateFontEntry(uniqueName, 
        gfxWindowsFontType(isCFF ? GFX_FONT_TYPE_PS_OPENTYPE : GFX_FONT_TYPE_TRUETYPE) /*type*/, 
        PRUint32(aProxyEntry.mItalic ? FONT_STYLE_ITALIC : FONT_STYLE_NORMAL), 
        w, winUserFontData);

With this change, the testpages below should render just as they do in 3.5:

http://people.mozilla.org/~jdaggett/webfonts/gentium-test.html
http://people.mozilla.org/~jdaggett/tests/titilliumtest.html
http://people.mozilla.org/~jdaggett/webfonts/droidserif-test.html

The code in gfxWindowsDWriteFontEntry::LoadLocalFont is also wrong, you're confusing family name lookups with face name lookups.  GDI muddles the distinction, you can use a family name (e.g. Arial) or a fullname (e.g. Arial Bold) to lookup a font using CreateFontIndirect.  You need to mimic the logic in gfxWindowsGDIFontEntry::LoadLocalFont which consists of:

1. Lookup a font using the fullname
2. Dig into the name table of the matched font to verify that the font fullname matches correctly (GDI returns a default font when the face isn't found)

Looking over the DWrite API docs this doesn't look easy.  Maybe you could use the IDWriteGdiInterop::CreateFontFromLOGFONT call?

Done correctly, this reftest below will render the same way for both pages:

http://people.mozilla.org/~jdaggett/font-face/src-list-local-full.html
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full-ref.html

Comment 79

9 years ago
Where does this fit with the layers work (https://wiki.mozilla.org/Gecko:Layers) coming up from roc? Am i correct in assuming that this bug focuses on the hardware rendering side, and roc's work on a higher level component to enable more efficient use of it?
(Assignee)

Comment 80

9 years ago
(In reply to comment #79)
> Where does this fit with the layers work
> (https://wiki.mozilla.org/Gecko:Layers) coming up from roc? Am i correct in
> assuming that this bug focuses on the hardware rendering side, and roc's work
> on a higher level component to enable more efficient use of it?
They're not that closely related. Roc's (and Jeff/Mine) layers work is about surface retention and hardware based surface composition. This is about actual hardware based vector graphics rendering.
(Assignee)

Comment 81

9 years ago
(In reply to comment #78)
> Lengthy, useful comments from :jtd.
Thanks for investigating. I'll see what I can do to fix these issues.

Personally I think using the interop is a 'very bad idea'(tm). I really do think it's important to move away from GDI, using bits and pieces here and there seems like very bad engineering to me. We'll have to see how we can make DWrite do what we want in my opinion. If you have any ideas, please let me know.
(Assignee)

Comment 82

9 years ago
I've fixed both bugs you mentioned. Without using GDI interop. One thing I'm still pondering about is that right now I'm comparing to the "en-us" locale when comparing the fullnames. I wonder if that's the correct behavior or if we should look at some specific locale? Once I know this I'll upload a new patch.
gfxWindowsSurface.cpp contains a bunch of whitespace changes. Let's avoid that for now.

Comment 84

9 years ago
(In reply to comment #82)
> I've fixed both bugs you mentioned. Without using GDI interop. One thing I'm
> still pondering about is that right now I'm comparing to the "en-us" locale
> when comparing the fullnames. I wonder if that's the correct behavior or if we
> should look at some specific locale? Once I know this I'll upload a new patch.

The locale is 'en-us' most of the time but you shouldn't need to decide that.  Use the same logic as the existing code, pass the name table to gfxFontUtils::ReadCanonicalName which contains fallback logic as well (for the case where an en-us name doesn't exist).

With your new patch, do reftests pass?  Especially reftests in layout/reftests/font-face?

Comment 85

9 years ago
BTW, what sort of Ts/Tp numbers do you see?  What's the increase/decrease in time spent within UpdateFontList during startup?
(Assignee)

Comment 86

9 years ago
Created attachment 420525 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v4

Added a new thebes patch with the changes suggested by jtd.
Attachment #418640 - Attachment is obsolete: true
1. what's the call to IASetPrimitiveTopology() for?

2. The alignment of the parameters of the prototypes at the top of
cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
structure initialization happens at the end of the file (like other backends)
and you make these functions static (they should be), then you should be able
to avoid having the prototypes entirely.

3. A question about the 'unique' parameter to
_cairo_d2d_create_brush_for_pattern:
It's not clear to me why we need more than one version of a brush.

4. "We can't actually map this type of texture, so we create one in CPU memory,
and the". What type is "this type"?
(Assignee)

Comment 88

9 years ago
(In reply to comment #87)
> 1. what's the call to IASetPrimitiveTopology() for?
It prevents a debug warning if you have the D3D debug layer switched on. I can remove this without consequence if you prefer.

> 
> 2. The alignment of the parameters of the prototypes at the top of
> cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
> structure initialization happens at the end of the file (like other backends)
> and you make these functions static (they should be), then you should be able
> to avoid having the prototypes entirely.

I believe the prototypes are a good idea, if only to concentrate the documentation of the function headers in one place. We could put the structure initialization happen at the end (or like in other backends in the middle somewhere or wherever they felt like it :)), but personally I think my structure of the file is -much- more readable. The functions which are implemented are one of the most important things to know when looking at a backend, hence I think the top, concentrated with the rest of central class information, is a good idea.

I'm fine with making them static, obviously.
> 
> 3. A question about the 'unique' parameter to
> _cairo_d2d_create_brush_for_pattern:
> It's not clear to me why we need more than one version of a brush.
The unique parameter means for a Color Brush and the Bitmap Brush, it doesn't use the ones stored on the class. But it creates new ones (brush creation has overhead, so I try to cache the brushes). This is needed for masking where we have a source pattern and a mask pattern.

> 
> 4. "We can't actually map this type of texture, so we create one in CPU memory,
> and the". What type is "this type"?

Erm, the surface storage texture. We can't map that directly, that comment is unclear for historical reasons. I'll fix that.
(In reply to comment #88)
> (In reply to comment #87)
> > 1. what's the call to IASetPrimitiveTopology() for?
> It prevents a debug warning if you have the D3D debug layer switched on. I can
> remove this without consequence if you prefer.
> 

Just add a comment.

> > 
> > 2. The alignment of the parameters of the prototypes at the top of
> > cairo-d2d-surface.cpp is off. If you reorder the file so that the backend
> > structure initialization happens at the end of the file (like other backends)
> > and you make these functions static (they should be), then you should be able
> > to avoid having the prototypes entirely.
> 
> I believe the prototypes are a good idea, if only to concentrate the
> documentation of the function headers in one place. We could put the structure
> initialization happen at the end (or like in other backends in the middle
> somewhere or wherever they felt like it :)), but personally I think my
> structure of the file is -much- more readable. The functions which are
> implemented are one of the most important things to know when looking at a
> backend, hence I think the top, concentrated with the rest of central class
> information, is a good idea.

Fair enough.

> I'm fine with making them static, obviously.
> > 
> > 3. A question about the 'unique' parameter to
> > _cairo_d2d_create_brush_for_pattern:
> > It's not clear to me why we need more than one version of a brush.
> The unique parameter means for a Color Brush and the Bitmap Brush, it doesn't
> use the ones stored on the class. But it creates new ones (brush creation has
> overhead, so I try to cache the brushes). This is needed for masking where we
> have a source pattern and a mask pattern.
> 

Why can't we use the same brush for the source and the mask?
(Assignee)

Comment 90

9 years ago
(In reply to comment #89)
> 
> Why can't we use the same brush for the source and the mask?

If you look at cairo-d2d-surface.cpp:1694, you'll notice we push a layer with one brush, then we draw with another (the source pattern) and then pop that layer. I have no idea if it is valid to change the opacity brush for that layer around while it is in use (I mean, obviously it holds a reference, so we could release it, but using the shared brush means we would actually be changing it). Maybe it is, it might be worth investigating since that would mean in theory we could get away without the unique brush.
(Assignee)

Comment 92

9 years ago
(In reply to comment #91)
> It looks like these patches break the windows mobile/ce build. Have at the
> bottom of this:
> 
> http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unix&logfile=1262995531.1262999274.7126.gz&buildtime=1262995531&buildname=WinCE%20try%20hg%20build&fulltext=1#err3
This is correct. There's a bunch of changes required to fix this in the font code. Hence me needing the DX SDK on the try servers. Although I'm also currently trying to get a mobile build going locally.
(Assignee)

Comment 93

9 years ago
Created attachment 420988 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v8

Processed comments.
Attachment #418639 - Attachment is obsolete: true
Attachment #420988 - Flags: review?(tellrob)
(Assignee)

Updated

9 years ago
Attachment #420988 - Flags: review?(jmuizelaar)
(Assignee)

Comment 94

9 years ago
Created attachment 421021 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v5

An update to the thebes patch. Adjusted for the new, recently commited font list code by jfkthame. This should make the patch queue work against current trunk again.
Attachment #420525 - Attachment is obsolete: true
(Assignee)

Comment 95

9 years ago
Created attachment 421033 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v6

Patch fixes license headers and a bunch of include issues.
Attachment #421021 - Attachment is obsolete: true
(Assignee)

Comment 96

9 years ago
Created attachment 421034 [details] [diff] [review]
Widget Direct2D Code v3

With this patch and the latest Thebes one this should now no longer break the windows mobile build.
Attachment #418210 - Attachment is obsolete: true
Bas, Do you have a new build system patch that doesn't require the DX sdk coming?
(Assignee)

Comment 98

9 years ago
(In reply to comment #97)
> Bas, Do you have a new build system patch that doesn't require the DX sdk
> coming?

I do, currently doing some clean builds to test that, should be up here soon. It's been modified to check for the WinSDK version.
(Assignee)

Comment 99

9 years ago
Created attachment 421052 [details] [diff] [review]
Build System DWrite/Direct2D mods v3

Updated buildsystem patch to no longer require DirectX SDK. Just the Windows 7 SDK in order to have D2D/DWrite support.
Attachment #418211 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #421033 - Flags: review?(jfkthame)
Attachment #421033 - Flags: review?(jdaggett)
(Assignee)

Updated

9 years ago
Attachment #421034 - Flags: review?(jmathies)
(Assignee)

Updated

9 years ago
Attachment #421052 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 100

9 years ago
Created attachment 421150 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v7

Fix a bug where wrapping a cairo surface would not correctly call gfxASurface::Init.
Attachment #421033 - Attachment is obsolete: true
Attachment #421033 - Flags: review?(jfkthame)
Attachment #421033 - Flags: review?(jdaggett)
(Assignee)

Updated

9 years ago
Attachment #421150 - Flags: review?(jfkthame)
Attachment #421150 - Flags: review?(jdaggett)
Couple quick comments:

+    DWRITE_FONT_METRICS fontMetrics;
+    mFontFace->GetMetrics(&fontMetrics);
+    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight / fontMetrics.designUnitsPerEm) * mStyle.size;

In this and *many* other places you're ignoring errors coming back from DirectWrite API calls.  Font quality varies dramatically and with downloadable fonts there's no guarantee that the font data isn't poop on a stick, so you really, really need to check for errors and handle them rather than assuming these calls always succeed.

+gfxFontEntry *
+gfxDWriteFontList::LookupLocalFont(const gfxProxyFontEntry *aProxyEntry,
+                                   const nsAString& aFontName)
+{
+    HRESULT hr;
+    IDWriteFontCollection *systemFonts;
+    hr = DWriteFactory::Instance()->GetSystemFontCollection(&systemFonts);
+    NS_ASSERTION(SUCCEEDED(hr), "GetSystemFontCollection failed!");
+
+    nsString lookupName(aFontName);
+    ToLowerCase(lookupName);
+    IDWriteFont *foundFont = NULL;
+    for (PRUint32 i = 0; i < systemFonts->GetFontFamilyCount(); i++) {
+        IDWriteFontFamily *family;
+        HRESULT hr;
+        hr = systemFonts->GetFontFamily(i, &family);
+        if (FAILED(hr)) {
+            continue;
+        }
+        IDWriteLocalizedStrings *familyNames;
+        family->GetFamilyNames(&familyNames);
+        UINT32 idx = 0;
+        BOOL exists;
+        WCHAR familyNameChar[256];
+        familyNames->FindLocaleName(L"en-us", &idx, &exists);
+        familyNames->GetString(idx, familyNameChar, 256);
+        familyNames->Release();
+        nsString familyName(familyNameChar);
+        ToLowerCase(familyName);
+        for (PRUint32 c = 0; c < family->GetFontCount(); c++) {
+            IDWriteFont *font;

Iterating over all font faces on a user's system is not a performant way of doing this.  Any user with tons of fonts on their system (e.g. Stuart!) will die during local() lookups.  If DirectWrite doesn't provide a way of doing direct fullname lookups, just use GDI for the lookup and convert that to a DirectWrite face.  Not ideal but far less sucky than iterating over all font faces.
Widget d2d patch has lots of trailing CR's and doesn't apply cleanly.
(Assignee)

Comment 103

9 years ago
(In reply to comment #101)
> Iterating over all font faces on a user's system is not a performant way of
> doing this.  Any user with tons of fonts on their system (e.g. Stuart!) will
> die during local() lookups.  If DirectWrite doesn't provide a way of doing
> direct fullname lookups, just use GDI for the lookup and convert that to a
> DirectWrite face.  Not ideal but far less sucky than iterating over all font
> faces.
Well, I'm very much against doing any GDI API's. Simply because GDI will be deprecated at some point. And we want this to be the DirectWrite version. Not the GDI one. Optionally I could store some information in a hash table and use that for the lookup?

On a sidenote, there does not appear to be a measurable difference in performance between using GDI lookup and this. I could gather exact timings on this though. But I'm sure that using some sort of hash lookup would be faster ofcourse.
(Assignee)

Comment 104

9 years ago
Created attachment 421221 [details] [diff] [review]
Widget Direct2D Code v4

Fix widget patch whitespacing. I must've accidently normalized the line-endings in the patch. We have some inconsistent newlines in the widget directory.
Attachment #421034 - Attachment is obsolete: true
Attachment #421034 - Flags: review?(jmathies)
(Assignee)

Updated

9 years ago
Attachment #421221 - Flags: review?(jmathies)
(Assignee)

Comment 105

9 years ago
(In reply to comment #101)
> Couple quick comments:
> 
> +    DWRITE_FONT_METRICS fontMetrics;
> +    mFontFace->GetMetrics(&fontMetrics);
> +    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight /
> fontMetrics.designUnitsPerEm) * mStyle.size;
> 
> In this and *many* other places you're ignoring errors coming back from
> DirectWrite API calls.  Font quality varies dramatically and with downloadable
> fonts there's no guarantee that the font data isn't poop on a stick, so you
> really, really need to check for errors and handle them rather than assuming
> these calls always succeed.
In this case, as well as several others, our API's don't have facilities for returning failure. Meaning that these font metrics I'm returning are just as much uninitialized memory as the ones I'll be returning if I just return here. In theory I think DWrite should parse the font and user font loading, and therefor this -should- never fail. I'll add a bunch of more checks in other places though to make sure we don't miss any potential failures.
(In reply to comment #105)
> (In reply to comment #101)
> > Couple quick comments:
> > 
> > +    DWRITE_FONT_METRICS fontMetrics;
> > +    mFontFace->GetMetrics(&fontMetrics);
> > +    mMetrics->xHeight = ((gfxFloat)fontMetrics.xHeight /
> > fontMetrics.designUnitsPerEm) * mStyle.size;
> > 
> > In this and *many* other places you're ignoring errors coming back from
> > DirectWrite API calls.  Font quality varies dramatically and with downloadable
> > fonts there's no guarantee that the font data isn't poop on a stick, so you
> > really, really need to check for errors and handle them rather than assuming
> > these calls always succeed.
> In this case, as well as several others, our API's don't have facilities for
> returning failure. Meaning that these font metrics I'm returning are just as
> much uninitialized memory as the ones I'll be returning if I just return here.
> In theory I think DWrite should parse the font and user font loading, and
> therefor this -should- never fail. I'll add a bunch of more checks in other
> places though to make sure we don't miss any potential failures.

GetMetrics returns a HRESULT value.  Please check it.

"If the method succeeds, it returns S_OK. Otherwise, it returns an HRESULT error code."

http://msdn.microsoft.com/en-us/library/dd371011%28VS.85%29.aspx

Compare your code with the existing GDI version of GetMetrics.  That code checks for an error and tries to deal with it other ways if there's an error.  If all else fails, mark the font not valid.  Using uninitialized data is neither valid nor safe.

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxWindowsFonts.cpp#274

If you think there are places where we should be doing a better job of recovering from errors, propose a fix.
(In reply to comment #103)
> Well, I'm very much against doing any GDI API's. Simply because GDI will be
> deprecated at some point. And we want this to be the DirectWrite version. Not
> the GDI one. 

There is no requirement currently to rid our code of GDI use.  Sure, it would be nicer, but if it's the difference between creating simple code that uses a GDI API and a larger piece of code that is slower or takes up extra memory, we should use GDI.
Comment on attachment 421052 [details] [diff] [review]
Build System DWrite/Direct2D mods v3

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -740,22 +740,23 @@ EOF
>     # version, error out
>     AC_MSG_CHECKING([for Windows SDK being recent enough])
>     if $PERL -e "exit(0x$MOZ_WINSDK_TARGETVER > $MOZ_WINSDK_MAXVER)"; then
>         AC_MSG_RESULT("yes")
>     else
>         AC_MSG_RESULT("no")
>         AC_MSG_ERROR([You are targeting Windows version 0x$MOZ_WINSDK_TARGETVER, but your SDK only supports up to version $MOZ_WINSDK_MAXVER. Install and use an updated SDK, or target a lower version using --with-windows-version. See https://developer.mozilla.org/En/Windows_SDK_versions for more details on fixing this.])
>     fi
>-
>+    
>     AC_DEFINE_UNQUOTED(MOZ_WINSDK_TARGETVER,0x$MOZ_WINSDK_TARGETVER)
>     # Definitions matching sdkddkver.h
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_WS03, 0x05020000)
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_LONGHORN, 0x06000000)
>     AC_DEFINE_UNQUOTED(MOZ_NTDDI_WIN7, 0x06010000)
>+    
>     ;;

Looks like you have some unintentional extra whitespace here. Please clean that up.

Looks good otherwise, r=me with that fixed.
Attachment #421052 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 109

9 years ago
(In reply to comment #107)
> (In reply to comment #103)
> > Well, I'm very much against doing any GDI API's. Simply because GDI will be
> > deprecated at some point. And we want this to be the DirectWrite version. Not
> > the GDI one. 
> 
> There is no requirement currently to rid our code of GDI use.  Sure, it would
> be nicer, but if it's the difference between creating simple code that uses a
> GDI API and a larger piece of code that is slower or takes up extra memory, we
> should use GDI.
Hrm, I personally think this is one of those cases where at some point in the future, part of the API gets removed, or changed, or might not work anymore as expected, and we end up going all over the tree having to fix it. I personally still favor a DWrite only solution (with a little hash table storing the fullname and the family & face index. That would be the fullname + two unsigned ints per font face as additional memory usage). Or at least doing exact timings to see if that GDI interop is any faster(a bunch of them are incredibly slow in many cases, DirectWrite makes use of a cross-process font cache, whereas GDI does not, I'm not sure how the interop will behave in this case).

As for the uninitialized font metrics. I'll put in the same behavior as GDI has which seems to make sense. I don't think the API actually ever fails (loading custom fonts returns an error code if it doesn't understand the font file), but you may very well be right and that may not check for valid metrics.
(Assignee)

Comment 110

9 years ago
(In reply to comment #109)
> (In reply to comment #107)
> > (In reply to comment #103)
> As for the uninitialized font metrics. I'll put in the same behavior as GDI has
> which seems to make sense. I don't think the API actually ever fails (loading
> custom fonts returns an error code if it doesn't understand the font file), but
> you may very well be right and that may not check for valid metrics.


Scratch that... I just noticed why there wasn't a check in there when trying to add one. The documentation on MSDN is just wrong, dwrite.h:1013 has that method returning void. So it cannot actually fail. I replaced the assertion on creating the FontFace with setting mIsValid to PR_FALSE though, since I suspect it might detect font corruption upon creating the font face.

Comment 111

9 years ago
(In reply to comment #103)
> Well, I'm very much against doing any GDI API's.

There could be also other reasons for sticking to the GDI API when dealing with fonts:
http://blog.getpaint.net/2009/12/21/paintnet-v352-is-on-the-way-%E2%80%93-better-font-support-for-win7vista/
Comment on attachment 421150 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v7

A few initial comments from skimming through the patch... (mostly little nits that happened to catch my eye, as I haven't really understood a lot of the actual code yet!)

> -EXPORTS +=	gfxWindowsFonts.h 
> +EXPORTS +=  gfxWindowsGDIFonts.h
> +EXPORTS +=  gfxWindowsDWriteFonts.h
> +EXPORTS +=  gfxD2DSurface.h

It might make merging a little bit simpler if you don't rename the existing gfxWindowsFonts files and classes (which will be radically restructured in bug 502906 part 2 anyway), though I suppose that can be handled with a few global-replace operations there.

Would it be OK to drop the "Windows" from names such as gfxWindowsDWriteFonts? I'd go for gfxDWriteFonts or maybe gfxDirectWriteFonts instead.

(For comparison, my current intention in bug 502906 aims to replace gfxWindowsFonts.* with gfxGDIFontAccessor.* and gfxUniscribeShaper.* ... I'd anticipate similarly having gfxDWriteFontAccessor and gfxDWriteShaper, eventually. Including "Windows" in all these names makes for excessive verbosity, IMO.)


> + * The Initial Developer of the Original Code is Oracle Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2005
> + * the Initial Developer. All Rights Reserved.

The occurrence of Oracle here surprised me, is that correct? The other files say Mozilla Foundation.

Also the copyright date presumably needs updating (similarly in all files).

> diff --git a/gfx/thebes/public/gfxWindowsSurface.h 
...
> +#include "nsPoint.h"
> +#include "nsRect.h"

Do we really need these added to the header, or can they just be included by the .cpp file if required there?


>  	   gfxDDrawSurface.cpp \
> -	   gfxFT2FontList.cpp \
> +	   gfxFT2FontList.cpp
>  	   $(NULL)

Removal of the continuation-backslash here is a mistake.


> +// Singleton accessor for DirectWriteFactory. Threadsafe. This
> +// will obviously leak the factory at shutdown.

I think we normally try to clean up stuff like this; could you hook in to the platform Shutdown method to do this, or the platform font-list destructor, or something?


> +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC);

Should also check for DWRITE_FONT_STYLE_OBLIQUE, I think. (We don't currently distinguish Oblique and Italic properly, though we should.....)


> +    UINT32 idx = 0;
> +    BOOL exists;
> +    systemFonts->FindFamilyName(L"tahoma", &idx, &exists);

I was surprised to see "tahoma" here, is that really the appropriate default?

What happens if it doesn't exist? The code doesn't seem to check the result in 'exists'.


> +            IDWriteLocalizedStrings *faceNames;
> +            font->GetFaceNames(&faceNames);
> +            UINT32 index = 0;
> +            /**
> +             * Use the English Locale, when not available index will remain
> +             * 0 and we will use the first name in the table.
> +             */
> +            faceNames->FindLocaleName(L"en-us", &index, &exists);
> +            faceNames->GetString(index, faceNameChar, 256);

This is wrong; MSDN doc for FindLocaleName says that "If the specified locale name does not exist, the return value is S_OK, but index is UINT_MAX and exists is FALSE." So you need to check the result of exists, and reset index if false.

> +        UINT32 idx = 0;
> +        BOOL exists;
> +        names->FindLocaleName(L"en-us", &idx, &exists);
> +        WCHAR famName[256];
> +        names->GetString(idx, famName, 256);

Ditto.


> +    // Call each of the analyzers in sequence, recording their results.
> +    if (SUCCEEDED(hr = textAnalyzer->AnalyzeLineBreakpoints(   this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeBidi(              this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeScript(            this, 0, mTextLength, this))
> +    &&  SUCCEEDED(hr = textAnalyzer->AnalyzeNumberSubstitution(this, 0, mTextLength, this))
> +    )

I believe Mozilla style says to break AFTER the operator such as &&.


> +{
> +    if (textLength > 0)
> +    {
> +        memcpy(mBreakpoints + textPosition, lineBreakpoints, textLength * sizeof(DWRITE_LINE_BREAKPOINT));
> +    }
> +    return S_OK;
> +}

There are a number of instances here where bracing style needs to be fixed; Mozilla style calls for the opening brace on the same line as the condition.

> +    if (*textLength < mCurrentRun->mTextLength)
> +    {
> +        SplitCurrentRun(mCurrentRun->mTextStart + *textLength);
> +    }
> +    else
> +    {
> +        // Just advance the current run.
> +        mCurrentRun = mCurrentRun->nextRun;
> +    }

....and "else" goes on the same line as the braces, too.


> +    // IUnknown interface
> +    IFACEMETHOD(QueryInterface)(IID const& iid, OUT void** ppObject)
> +    {
> +        if (iid == __uuidof(IDWriteTextAnalysisSource)) {
> +	    *ppObject = static_cast<IDWriteTextAnalysisSource*>(this);

Indentation.

> +cairo_font_face_t *
> +gfxWindowsDWriteFont::CairoFontFace()
> +{
> +    if (!mCairoFontFace) {
> +#ifdef CAIRO_HAS_DWRITE_FONT
> +        mCairoFontFace = 
> +	    cairo_dwrite_font_face_create_for_dwrite_fontface(
> +	    ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> +#endif

Indentation.

Oh, and we normally use static_cast<> etc rather than C-style casts. I've probably been overlooking those up till now.

> +    gfxWindowsDWriteFontGroup *fontGroup =
> +        (gfxWindowsDWriteFontGroup*)closure;

static_cast<>.

> +        entry = (gfxWindowsDWriteFontEntry*)fontGroup->mUserFontSet->FindFontEntry(aName, fontGroup->mStyle, needsBold);

static_cast, and excessive line length.

> +        if (searchFont->GetStyle() != style || searchFont->GetStretch() != DWriteFontStretchFromStretch(fontGroup->mStyle.stretch)) {

Line length.

> +    if (mFontEntries.Length() == 0) {
> +        // It is pretty important that we have at least one font, so
> +        // try a few system fonts that should be there.
> +        nsAutoString str;
> +        HGDIOBJ hGDI = ::GetStockObject(DEFAULT_GUI_FONT);
> +        LOGFONTW logFont;
> +        if (hGDI && ::GetObjectW(hGDI, sizeof(logFont), &logFont)) {
> +            str.AppendLiteral("\"");
> +            str.Append(nsDependentString(logFont.lfFaceName));
> +            str.AppendLiteral("\"");
> +        }

Is there no DWrite equivalent to the GDI usage here? It seems unfortunate to mix the two, if it's possible to avoid.

> +gfxTextRun *
> +gfxWindowsDWriteFontGroup::MakeTextRun(const PRUint8 *aString, PRUint32 aLength,
> +                                   const Parameters *aParams, PRUint32 aFlags)
> +{
> +    nsDependentCString string((const char*)aString);

Offhand I'm not sure whether aString is guaranteed to be null-terminated; should you pass in aLength here, to be on the safe side?

> +	
>          mContext->Save();
> +	mContext->SetAntialiasMode(gfxContext::MODE_ALIASED);
>          mContext->Translate(mNativeRect.pos);
>          mContext->NewPath();
> -        mContext->Rectangle(gfxRect(gfxPoint(0.0, 0.0), mNativeRect.size));
> +        mContext->Rectangle(gfxRect(gfxPoint(0.0, 0.0), 
> +	    gfxSize(NS_ceil(mNativeRect.size.width), NS_ceil(mNativeRect.size.height)
> +	    )));
>  
>          nsRefPtr<gfxPattern> pat = new gfxPattern(alphaSurface);
>  
> +	pat->SetFilter(gfxPattern::FILTER_NEAREST);

Indentation looks strange; aha, I just realized there are "real" tabs here. Please expand to spaces. That also explains some earlier "indentation" issues.
(Assignee)

Comment 113

9 years ago
(In reply to comment #112)
> (From update of attachment 421150 [details] [diff] [review])
I replied to the things that weren't completely obvious :). Thanks for the quick feedback, my appologies on the tabs space mixes.. Just to explain:

Widget: Spaces, indent 2
Thebes: Spaces, indent 4
Cairo: Tabs, tab-width 8, indent 4

I must've forgotten to change modes a couple of times.

> It might make merging a little bit simpler if you don't rename the existing
> gfxWindowsFonts files and classes (which will be radically restructured in bug
> 502906 part 2 anyway), though I suppose that can be handled with a few
> global-replace operations there.
> 
> Would it be OK to drop the "Windows" from names such as gfxWindowsDWriteFonts?
> I'd go for gfxDWriteFonts or maybe gfxDirectWriteFonts instead.

I'm fine with all that, you guys decide :) I'd go for DWrite though. Since that's what MS uses everywhere.
> > + * The Initial Developer of the Original Code is Oracle Corporation.
> > + * Portions created by the Initial Developer are Copyright (C) 2005
> > + * the Initial Developer. All Rights Reserved.
> 
> The occurrence of Oracle here surprised me, is that correct? The other files
> say Mozilla Foundation.
> 
> Also the copyright date presumably needs updating (similarly in all files).

Erm, yes, I need to fix up the license plates, that Oracle thing is a big Oops. Will fix this.

> 
> > diff --git a/gfx/thebes/public/gfxWindowsSurface.h 
> ...
> > +#include "nsPoint.h"
> > +#include "nsRect.h"
> 
> Do we really need these added to the header, or can they just be included by
> the .cpp file if required there?
Good point, no they don't. They're from the old D2D code that was in there a couple of iterations back.

> 
> 
> >  	   gfxDDrawSurface.cpp \
> > -	   gfxFT2FontList.cpp \
> > +	   gfxFT2FontList.cpp
> >  	   $(NULL)
> 
> Removal of the continuation-backslash here is a mistake.
Heh, fixed that on my mobile test build, forgot to merge back to my normal D2D tree.
> 
> 
> > +// Singleton accessor for DirectWriteFactory. Threadsafe. This
> > +// will obviously leak the factory at shutdown.
> 
> I think we normally try to clean up stuff like this; could you hook in to the
> platform Shutdown method to do this, or the platform font-list destructor, or
> something?

Robarnold made a good suggestion here for what we can do I think. I'll look into doing that.
> 
> 
> > +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC);
> 
> Should also check for DWRITE_FONT_STYLE_OBLIQUE, I think. (We don't currently
> distinguish Oblique and Italic properly, though we should.....)
> 
> 
> > +    UINT32 idx = 0;
> > +    BOOL exists;
> > +    systemFonts->FindFamilyName(L"tahoma", &idx, &exists);
> 
> I was surprised to see "tahoma" here, is that really the appropriate default?
> 
> What happens if it doesn't exist? The code doesn't seem to check the result in
> 'exists'.
IDX will remain 0, so it will take the first font in the font list. Tahoma is the default font according to http://msdn.microsoft.com/en-us/library/dd144925%28VS.85%29.aspx. (we don't have a DEFAULT_GUI_FONT thing that I can find in DWrite, maybe we could use the Theme API's somehow).
> 
> 
> > +            IDWriteLocalizedStrings *faceNames;
> > +            font->GetFaceNames(&faceNames);
> > +            UINT32 index = 0;
> > +            /**
> > +             * Use the English Locale, when not available index will remain
> > +             * 0 and we will use the first name in the table.
> > +             */
> > +            faceNames->FindLocaleName(L"en-us", &index, &exists);
> > +            faceNames->GetString(index, faceNameChar, 256);
> 
> This is wrong; MSDN doc for FindLocaleName says that "If the specified locale
> name does not exist, the return value is S_OK, but index is UINT_MAX and exists
> is FALSE." So you need to check the result of exists, and reset index if false.
Well spotted :-).
> > +    if (mFontEntries.Length() == 0) {
> > +        // It is pretty important that we have at least one font, so
> > +        // try a few system fonts that should be there.
> > +        nsAutoString str;
> > +        HGDIOBJ hGDI = ::GetStockObject(DEFAULT_GUI_FONT);
> > +        LOGFONTW logFont;
> > +        if (hGDI && ::GetObjectW(hGDI, sizeof(logFont), &logFont)) {
> > +            str.AppendLiteral("\"");
> > +            str.Append(nsDependentString(logFont.lfFaceName));
> > +            str.AppendLiteral("\"");
> > +        }
> 
> Is there no DWrite equivalent to the GDI usage here? It seems unfortunate to
> mix the two, if it's possible to avoid.
I could do the same trick here mentioned earlier for the default system font.
(Assignee)

Comment 114

9 years ago
(In reply to comment #113)
> (In reply to comment #112)
> > (From update of attachment 421150 [details] [diff] [review] [details])
> IDX will remain 0, so it will take the first font in the font list. Tahoma is
> the default font according to
> http://msdn.microsoft.com/en-us/library/dd144925%28VS.85%29.aspx. (we don't
> have a DEFAULT_GUI_FONT thing that I can find in DWrite, maybe we could use the
> Theme API's somehow).

Obviously I was plain wrong here, it will become UINT_MAX just like in the other case you spotted. Will fix those.
(Assignee)

Comment 115

9 years ago
Created attachment 421283 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v8

Patch with majority of jfkthame's preliminary comments. (no file/class renames yet)
Attachment #421150 - Attachment is obsolete: true
Attachment #421283 - Flags: review?(jfkthame)
Attachment #421283 - Flags: review?(jdaggett)
Attachment #421150 - Flags: review?(jfkthame)
Attachment #421150 - Flags: review?(jdaggett)
Comment on attachment 420988 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v8

1. _cairo_dwrite_scaled_font_init_glyph_surface has the wrong argument alignment and has some pretty dense blocks of code. Adding some line breaks will help here.

2. _cairo_d2d_flush is not hooked up and should be.

3. We assert on GetFirstMatchingFont failing. We should probably handle the error instead.

Other than that it looks good enough to get into the tree. We can make further improvements when it's there.
Attachment #420988 - Flags: review?(jmuizelaar) → review+
Created attachment 421395 [details]
Font problem

You use "en-us" locale at gfxWindowsDWriteFontFamily::FindStyleVariations,
gfxDWriteFontList::LookupLocalFont, gfxDWriteFontList:InitFontList.
These cause wrong font choosing in environment other than "en-us".

you don't use "en-us", get user locale with "GetUserDefaultLocaleName".
(In reply to comment #117)
> Created an attachment (id=421395) [details]
> Font problem
> 
> You use "en-us" locale at gfxWindowsDWriteFontFamily::FindStyleVariations,
> gfxDWriteFontList::LookupLocalFont, gfxDWriteFontList:InitFontList.
> These cause wrong font choosing in environment other than "en-us".
> 
> you don't use "en-us", get user locale with "GetUserDefaultLocaleName".

The LookupLocalFont method is used to lookup fonts by their fullname in @font-face rules.  As such, it should only use the en-us name or the first locale name if an en-us version does not exist.  This is used as a canonical fullname to avoid interoperability problems with large numbers of localizations of style names like bold, italic, etc.

But any user-facing name should be using the locale-specific name as noted.
notice dialog's font. d2d ver choise wrong font.
(In reply to comment #119)
> notice dialog's font. d2d ver choise wrong font.

Right, definitely wrong.  My comment only pertains to the use of the 'en-us' locale in LookupLocalFont.
Looks like synthetic rendering for downloadable fonts is busted too:

http://people.mozilla.org/~jdaggett/font-face/synthetic-variations.html

Should see plain, fake bold, fake italic, fake bold/italic in the four columns.  Instead all four columns use plain face, no fake nuttin.  Compare to pre-1/8 trunk builds.
(Assignee)

Comment 122

9 years ago
Created attachment 421562 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v9

Processed latest jrmuizel comments.
(Assignee)

Updated

9 years ago
Attachment #420988 - Attachment is obsolete: true
Attachment #420988 - Flags: review?(tellrob)
(Assignee)

Updated

9 years ago
Attachment #421562 - Flags: review?(tellrob)
Attachment #421562 - Flags: review?(jmuizelaar)
(Assignee)

Comment 123

9 years ago
Created attachment 421563 [details] [diff] [review]
Build System DWrite/Direct2D mods v4
Attachment #421052 - Attachment is obsolete: true
Attachment #421563 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #421563 - Attachment description: Build System DWrite/Direct2D mods v3 → Build System DWrite/Direct2D mods v4
(Assignee)

Updated

9 years ago
Attachment #421563 - Flags: review? → review?(ted.mielczarek)
+already_AddRefed<gfxWindowsDWriteFont>
+gfxWindowsDWriteFont::GetOrMakeFont(gfxFontEntry *aFontEntry,
+				    const gfxFontStyle *aStyle,
+				    PRBool aNeedsBold)
+{
+    // because we know the FontEntry has the weight we really want, use it for matching
+    // things in the cache so we don't end up with things like 402 in there.
+    gfxFontStyle style(*aStyle);
+
+    if (aFontEntry->mIsUserFont && !aFontEntry->IsBold()) {
+        // determine whether synthetic bolding is needed
+        PRInt8 baseWeight, weightDistance;
+        aStyle->ComputeWeightAndOffset(&baseWeight, &weightDistance);
+
+        if ((weightDistance == 0 && baseWeight >= 6) || (weightDistance > 0 && aNeedsBold)) {
+            style.weight = 700;  
+        } else {
+            style.weight = aFontEntry->mWeight;
+        }
+    } else {
+        style.weight = aFontEntry->mWeight;
+    }
+    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aFontEntry->Name(), aStyle);
+
+    if (!font) {
+        font = new gfxWindowsDWriteFont(static_cast<gfxWindowsDWriteFontEntry*>(aFontEntry), aStyle);
+        if (!font) {
+            return nsnull;
+        }
+        gfxFontCache::GetCache()->AddNew(font);
+    }
+
+    font->AddRef();
+    return static_cast<gfxWindowsDWriteFont*>(font.get());
+}

After making a local copy of the style, you don't use it.  You need to pass &style instead of aStyle to the cache lookup method and the constructor.
(Assignee)

Comment 125

9 years ago
(In reply to comment #124)
> 
> After making a local copy of the style, you don't use it.  You need to pass
> &style instead of aStyle to the cache lookup method and the constructor.

Indeed, I changed them back to aStyle to test another font lookup issue that I created with my new code. And forgot to change them back when I went back to the synthetic bolding issues. Thanks a lot for helping me find that one.
Also looks like italics are incorrect for normal fonts (i.e. not @font-face fonts).  Italic fonts are rendering with synthetic italics rather than the real italic face.

Compare these:
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full.html
http://people.mozilla.org/~jdaggett/font-face/src-list-local-full-ref.html

The -ref version is incorrect, it's obliquing the regular face rather than using the italic face.
Comment on attachment 421283 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v8

Maybe it's just paranoia, given how many problems we've seen caused by bad fonts and unpredictable platform API response to them, but I'd really like to see error checking added in places like this.... if the DWrite APIs return result codes, don't ignore them.

>+void
>+gfxWindowsDWriteFontFamily::FindStyleVariations()
>+{
>+    mAvailableFonts.SetLength(mDWFamily->GetFontCount());
>+
>+    for (UINT32 i = 0; i < mDWFamily->GetFontCount(); i++) {
>+        IDWriteFont *font;
>+        mDWFamily->GetFont(i, &font);

Check for failure. I know it shouldn't occur, but weird things happen when users install random collections of "1000 fonts for $10", and most of them are flaky.....

(And if error occurs, log a warning (in debug builds) and skip this face; we just won't use it.)

>+        IDWriteLocalizedStrings *names;
>+        font->GetFaceNames(&names);

Ditto.

>+        WCHAR faceName[256];
>+        UINT32 idx = 0;
>+        BOOL exists;
>+        names->FindLocaleName(L"en-us", &idx, &exists);

Check result for failure; check 'exists'. It is not guaranteed that a font has en-us names! (Exceptions are very rare, but possible.)

>+        names->GetString(idx, faceName, 256);

Check result for failure.

Don't use a fixed-size buffer and hard-coded length limit; use GetStringLength to resize a stack-based nsAutoString or nsAutoTArray with an auto-size large enough for normal names (so there's no actual allocation cost except in pathological cases). In principle, names in OpenType can be up to 64K long (although I'm sure plenty of other software would fail on them too!)

>+        names->Release();
>+        nsString fullName(mName);
>+        fullName.AppendLiteral(" ");
>+        fullName.Append(faceName);

I wish DWrite gave us access to the real fullname from the font. In the absence of that, you should probably check whether the faceName is "Regular", and if so, don't append it. (The OpenType spec says, "Full font name; this should be a combination of strings 1 and 2. Exception: if the font is “Regular” as indicated in string 2, then use only the family name contained in string 1.")

>+        gfxWindowsDWriteFontEntry *fe = new gfxWindowsDWriteFontEntry(fullName, font);
>+        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
>+                       font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
>+        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
>+        fe->mWeight = font->GetWeight();

It's not entirely clear from the DWrite documentation whether GetWeight() can ONLY return defined values from the DWRITE_FONT_WEIGHT enumeration, or if it returns whatever integer it finds in the font. CSS weights, OTOH, are only allowed to be round hundreds in the range 100..900. So you should at least clamp the result to this range (i.e. we can't accept the DWRITE_FONT_WEIGHT_EXTRA_BLACK = 950 value here), and it might be wisest to explicitly round to the nearest multiple of 100. (Either that, or create a test font with an intermediate usWeight value, and check how DWrite actually handles it.)

>+        mAvailableFonts[i] = fe;

Append the entries rather than stuffing them into fixed locations, to allow for potential skipped faces if the calls above return failure.

>+    }
>+}

There'll be similar error-handling considerations in some other places....
(Assignee)

Comment 128

9 years ago
(In reply to comment #127)
> (From update of attachment 421283 [details] [diff] [review])
> Maybe it's just paranoia, given how many problems we've seen caused by bad
> fonts and unpredictable platform API response to them, but I'd really like to
> see error checking added in places like this.... if the DWrite APIs return
> result codes, don't ignore them.
> 

I honestly think that in many of these cases they return result codes to indicate API usage errors (i.e. E_POINTER) or things like that. If we're sure to ask for an index smaller than GetFontCount() I honestly don't think it can -actually- fail. I believe DirectWrite will have thrown bad fonts out already by then. (for example, if we read a user font we will be calling an 'analyze' function first which returns an 'isSupported' bool) I think adding actual checks on all of them would be a waste of countless conditional branches. I'd prefer to add assertions there so we'll know when maybe that assumption turns out to be wrong, but I doubt it, and I really don't think adding all these checks is very benificial. Something similar goes for GetNames and such.
(Assignee)

Comment 129

9 years ago
Created attachment 421623 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v9

Process comments from jfkthame and jdaggett. Should fix all problems listed here.
Attachment #421283 - Attachment is obsolete: true
Attachment #421283 - Flags: review?(jfkthame)
Attachment #421283 - Flags: review?(jdaggett)
Attachment #421562 - Flags: review?(jmuizelaar) → review+
Attachment #421563 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

9 years ago
Attachment #421623 - Flags: review?(jfkthame)
Attachment #421623 - Flags: review?(jdaggett)
Comment on attachment 421623 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v9

>+ * The Initial Developer of the Original Code is Oracle Corporation.

Hmm, this didn't get fixed yet...

>+    for (UINT32 i = 0; i < mDWFamily->GetFontCount(); i++) {
>+        IDWriteFont *font;
>+        hr = mDWFamily->GetFont(i, &font);
>+	if (FAILED(hr)) {
>+	    // This should never happen.
>+	    NS_WARNING("Failed to get existing font from family.");
>+	    continue;
>+	}

Oops, your editor was in the wrong mode - real tabs here. (Several times in this function.)

>+        if (!exists) {
>+            englishIdx = 0;
>+        }

Perhaps add a comment about falling back to whatever language is first in the list, as "englishIdx" is not in fact English in this case?

>+        hr = names->GetString(englishIdx, faceName, 256);

It'd still be better not to use a hard-coded size limit, IMO.

>+	nsString fullNameCase(mName);
>+	fullNameCase.AppendLiteral(" ");
>+	fullNameCase.Append(faceName);

Would it be appropriate to skip appending "Regular" here, too? And in that case, you could simply lowercase this string later for the fullname table, rather than having to construct a second version of it. I can't think of a reason you'd need a to maintain a different form of the name here.

>+	PRUint16 weight = font->GetWeight();
>+	// Round to nearest 100.
>+	weight += 50;
>+	weight /= 100;
>+	weight *= 100;
>+	if (weight < 100) {
>+	    weight = 100;
>+	}
>+	if (weight > 900) {
>+	    weight = 900;
>+	}

I've realized we have some convenience macros you could use here, to make the code easier to read: how about

  PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
  weight = PR_MAX(100, weight);
  weight = PR_MIN(900, weight);

The resulting code should be equivalent, but it's a lot more concise here in the source.

>+        fe->mWeight = font->GetWeight();

Oops, having sanitized the weight value, be sure to use it!

>+        nsString faceNameString(faceName);
>+        nsString fullName(mName);
>+        ToLowerCase(faceNameString);
>+        ToLowerCase(fullName);
>+        if (!faceNameString.EqualsLiteral("regular") &&
>+            !faceNameString.EqualsLiteral("normal")) {
>+		fullName.AppendLiteral(" ");
>+		fullName.Append(faceNameString);
>+                // Put this in as the 'standard' face.
>+        }

See above, can you just reuse the same "fullname" (lowercased) here?

>+	if (font->GetStyle() != DWRITE_FONT_STYLE_OBLIQUE) {
>+	    /**
>+	     * Oblique fonts don't go in here, since we cannot distinquish them
>+	     * from italic with the current gfxFontEntry structure.
>+	     */
>+            mAvailableFonts.AppendElement(fe);
>+	}

I think we'd only want to omit oblique fonts here if the family ALSO has separate italic faces; if it only has upright and oblique, then we do want to use the oblique. (It's better to use a true oblique face, if available, than to synthesize one.)

Which means, I suppose, that you have to collect all the faces, and then filter out the oblique ones only if they are "shadowed" by italics. (Offhand, I can only think of a handful of families that actually have both.)

>+    }
>+}

Before returning from FindStyleVariations, I think it would be good to check that at least one face was successfully added to mAvailableFonts, and at least issue an NS_WARNING if not. (Ideally, I think we should remove any such "broken" families from our font list, but I don't think any of the platforms currently handle that.)


>+gfxFontEntry *
>+gfxDWriteFontList::GetDefaultFont(const gfxFontStyle *aStyle,
>+                                  PRBool &aNeedsBold)

The error-handling code here also has tabs. :(


>+    /**
>+     * TODO: This may not work correctly if this is accessed before
>+     * FindStyleVariations has been called on all families. We may be able
>+     * to "accelerate" runloader if it is needed here.
>+     */

I think we do need to handle this, otherwise src:local(...) will fail if it's on the startup page (or, for example, a page that's open in a session that's being restored when the browser launches).

One way to mitigate the impact of that somewhat would be to separate the loading of cmaps from the population of font families with their faces; we only need to populate the families in order to do name lookups, so cmap reading could be deferred to a second stage of font-loader operation.


>+gfxFontEntry *
>+gfxDWriteFontList::MakePlatformFont(const gfxProxyFontEntry *aProxyEntry,
>+                                    const PRUint8 *aFontData,
>+                                    PRUint32 aLength)

This function is responsible for ensuring aFontData gets deleted when it is no longer needed. (You could probably just borrow the stack-based helper approach from the gfxGDIFontList implementation.)

>+    // Postscript-style glyphs, swizzle name table, load directly

This comment is no longer true, as you're loading all fonts this way regardless of the type of glyph data.

>+    PRUint16 w = (aProxyEntry->mWeight == 0 ? 400 : aProxyEntry->mWeight);
>+    gfxWindowsDWriteFontEntry *entry = new gfxWindowsDWriteFontEntry(uniqueName, NULL, fontFile);
>+    entry->mItalic = aProxyEntry->mItalic;
>+    entry->mWeight = aProxyEntry->mWeight;

This should presumably be w, not aProxyEntry->mWeight.

>+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
>+    if (!isSupported) {
>+        delete entry;
>+        return nsnull;
>+    }

Would it be ok to call Analyze() and check isSupported before creating the new fontEntry?


>+void
>+gfxDWriteFontList::InitFontList()
>+{
>+    HRESULT hr;
>+    gfxFontCache *fc = gfxFontCache::GetCache();
>+    if (fc) {
>+        fc->AgeAllGenerations();
>+    }
>+    mFontFamilies.Clear();

Also need to clear the fullname table.

>+        WCHAR famName[256];
>+        WCHAR famLocalName[256];

I still don't much like the hard-coded sizes here. There should be no significant cost to using e.g. an nsAutoTArray<WCHAR,32> instead, and checking the actual length.
(In reply to comment #130)
>   PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
>   weight = PR_MAX(100, weight);
>   weight = PR_MIN(900, weight);
Don't do it!  You want to us NS_MIN and NS_MAX here, not the NSPR functions.  See bug 518502.
(Assignee)

Comment 132

9 years ago
(In reply to comment #130)
> (From update of attachment 421623 [details] [diff] [review])
> >+	if (font->GetStyle() != DWRITE_FONT_STYLE_OBLIQUE) {
> >+	    /**
> >+	     * Oblique fonts don't go in here, since we cannot distinquish them
> >+	     * from italic with the current gfxFontEntry structure.
> >+	     */
> >+            mAvailableFonts.AppendElement(fe);
> >+	}
> 
> I think we'd only want to omit oblique fonts here if the family ALSO has
> separate italic faces; if it only has upright and oblique, then we do want to
> use the oblique. (It's better to use a true oblique face, if available, than to
> synthesize one.)
> 
> Which means, I suppose, that you have to collect all the faces, and then filter
> out the oblique ones only if they are "shadowed" by italics. (Offhand, I can
> only think of a handful of families that actually have both.)

I think we need to spec the -exact- desired behavior here, i.e. what if we have a 200 oblique, a 400 italic, a 600 oblique and an 800 italic?

> >+    /**
> >+     * TODO: This may not work correctly if this is accessed before
> >+     * FindStyleVariations has been called on all families. We may be able
> >+     * to "accelerate" runloader if it is needed here.
> >+     */
> 
> I think we do need to handle this, otherwise src:local(...) will fail if it's
> on the startup page (or, for example, a page that's open in a session that's
> being restored when the browser launches).
> 
> One way to mitigate the impact of that somewhat would be to separate the
> loading of cmaps from the population of font families with their faces; we only
> need to populate the families in order to do name lookups, so cmap reading
> could be deferred to a second stage of font-loader operation.

Where do you think we should do this?
> 
> 
> >+gfxFontEntry *
> >+gfxDWriteFontList::MakePlatformFont(const gfxProxyFontEntry *aProxyEntry,
> >+                                    const PRUint8 *aFontData,
> >+                                    PRUint32 aLength)
> 
> This function is responsible for ensuring aFontData gets deleted when it is no
> longer needed. (You could probably just borrow the stack-based helper approach
> from the gfxGDIFontList implementation.)

I only have one additional return point when I need to NS_Free it. So I'll just to it manually.
> >+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
> >+    if (!isSupported) {
> >+        delete entry;
> >+        return nsnull;
> >+    }
> 
> Would it be ok to call Analyze() and check isSupported before creating the new
> fontEntry?
Well, otherwise we'd have to store the facetype in a temp and then assign it to the font entry member (since it we pass it directly into analyze now), but we could do it. I don't see a real point though, failure should be the rare case, and a deallocation isn't that expensive. Let me know what you think we should do.
(In reply to comment #131)
> (In reply to comment #130)
> >   PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
> >   weight = PR_MAX(100, weight);
> >   weight = PR_MIN(900, weight);
> Don't do it!  You want to us NS_MIN and NS_MAX here, not the NSPR functions. 
> See bug 518502.

Oops. Right.

In that case, it might be nice to create an NS_ROUND (and/or NS_ROUNDUP) too, though PR_ROUNDUP is only used in a few places.
(In reply to comment #132)
> I think we need to spec the -exact- desired behavior here, i.e. what if we have
> a 200 oblique, a 400 italic, a 600 oblique and an 800 italic?

The CSS Fonts spec describes the matching algorithm we're suppose to use (which  gfxFontFamily::FindFontForStyle is responsible to implement). So the font list should just collect all the faces in the family, and let that function make the choices.

Our gfxFontStyle actually supports the italic/oblique distinction, IIRC; maybe it's time to fix the gfxFontEntry so that it can, too.


> > One way to mitigate the impact of that somewhat would be to separate the
> > loading of cmaps from the population of font families with their faces; we only
> > need to populate the families in order to do name lookups, so cmap reading
> > could be deferred to a second stage of font-loader operation.
> 
> Where do you think we should do this?

We could modify the font info loader so that it loops over the families twice instead of once, loading styles the first time and cmaps the second time; then implement a "ForceLoadStyles" function that ensures the first loop has been completed.


> > This function is responsible for ensuring aFontData gets deleted when it is no
> > longer needed. (You could probably just borrow the stack-based helper approach
> > from the gfxGDIFontList implementation.)
> 
> I only have one additional return point when I need to NS_Free it. So I'll just
> to it manually.

Fine, if that's most convenient.


> > Would it be ok to call Analyze() and check isSupported before creating the new
> > fontEntry?
> Well, otherwise we'd have to store the facetype in a temp and then assign it to
> the font entry member (since it we pass it directly into analyze now), but we
> could do it. I don't see a real point though, failure should be the rare case,
> and a deallocation isn't that expensive. Let me know what you think we should
> do.

I don't suppose it really matters. (In the failure case, it would avoid both the allocation and deallocation, of course, but it's still not a big deal.)
A few more comments, then I'm away for the week so you'll have some peace! :)

> +    // We do not use this for now, store anyway
> +    DWRITE_LINE_BREAKPOINT *mBreakpoints;

I'd suggest removing this and the code that uses it, as it's not being used; or disable it with an #ifdef USE_DWRITE_BREAKPOINTS for now. That will eliminate the allocation/deallocation of the (currently redundant) mBreakpoints array, and the call to AnalyzeLineBreakpoints. Keeping the code within an #ifdef will mean we can easily bring it back if we want to use DWrite's line-breaking at some point in the future.

> +	DWRITE_FONT_SIMULATIONS sims = DWRITE_FONT_SIMULATIONS_NONE;
> +	if ((GetStyle()->style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) &&
> +	    !fe->IsItalic()) {
> +		sims |= DWRITE_FONT_SIMULATIONS_OBLIQUE;
> +	}

Oops, more tabs.... please run a global search of some kind across the files to clean them all up.

> +gfxWindowsDWriteFont::~gfxWindowsDWriteFont()
> +{
> +    if (mFontFace) {
> +        mFontFace->Release();
> +    }
> +}

Does this need to free the Cairo fontface and scaledfont as well, or did I miss that somewhere?


> +nsString
> +gfxWindowsDWriteFont::GetUniqueName()
> +{
> +    nsString familyName = mFontEntry->FamilyName();
> +    nsString fontName = mFontEntry->Name();
> +    return familyName + NS_LITERAL_STRING(" ") + fontName;
> +}

I wonder if this should use the "fullname" rule of not appending "Regular"? Sorry, I ran out of time to look into where/how it's used....


> +void 
> +gfxWindowsDWriteFontGroup::UpdateFontList()
> +{
> +    // if user font set is set, check to see if font list needs updating
> +    if (mUserFontSet && mCurrGeneration != GetGeneration()) {
> +        // xxx - can probably improve this to detect when all fonts were found, so no need to update list
> +        mFonts.Clear();
> +        InitFontList();
> +        mCurrGeneration = GetGeneration();
> +    }
> +
> +}

I think this needs to do

+    mUnderlineOffset = UNDERLINE_OFFSET_NOT_SET;

as well, otherwise you're likely to run into bug 523717.


> +    /**
> +     * There's an internal 16-bit limit on some things inside the analyzer.
> +     * to be on the safe side here we split up any ranges over MAX_RANGE_LENGTH characters.
> +     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
> +     */

Do you know what could break if, for example, the total number of glyphs generated exceeds 64K? It's possible for the number of glyphs resulting from a text run to be several times greater than the number of characters. (E.g. some Urdu fonts are examples of this.)
(Assignee)

Comment 136

9 years ago
(In reply to comment #135)
> A few more comments, then I'm away for the week so you'll have some peace! :)
No problem! It's great to get quick reviews so I can fix up all this font code stuff. I knew in advance this would be the tricky part :-).

I can a search and replaced all tabs btw. Or atleast I hope I got them all.

> 
> > +    /**
> > +     * There's an internal 16-bit limit on some things inside the analyzer.
> > +     * to be on the safe side here we split up any ranges over MAX_RANGE_LENGTH characters.
> > +     * TODO: Figure out what exactly is going on, and what is a safe number, and why.
> > +     */
> 
> Do you know what could break if, for example, the total number of glyphs
> generated exceeds 64K? It's possible for the number of glyphs resulting from a
> text run to be several times greater than the number of characters. (E.g. some
> Urdu fonts are examples of this.)

Nothing will truly break, but part of the text run won't be rendered.
(Assignee)

Comment 137

9 years ago
Created attachment 422132 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v10

New DirectWrite code, also adjusted to latest changes to the font classes. Synthetic bolding for regular fonts still needs to be double checked. It probably won't work in all cases yet.
Attachment #421623 - Attachment is obsolete: true
Attachment #421623 - Flags: review?(jfkthame)
Attachment #421623 - Flags: review?(jdaggett)
Created attachment 422183 [details]
fon tproblem with thebes patch v10

font list show localized name, but still choice wrong font.
(Assignee)

Comment 139

9 years ago
(In reply to comment #138)
> Created an attachment (id=422183) [details]
> fon tproblem with thebes patch v10
> 
> font list show localized name, but still choice wrong font.

Hrm, odd, jdaggett any ideas? I wonder if this is the result of a bug jfkthame and I found in the old GDI code that somehow made things work. As far as I understand lookup should en-us locales so localization should -only- affect how fonts are displayed, not how they are chosen.
I tried to make mod ver that replace 'en-us' to localeName in FindStyleVariations and InitFontList. Mod ver Fx select right font.
(Assignee)

Comment 141

9 years ago
(In reply to comment #140)
> I tried to make mod ver that replace 'en-us' to localeName in
> FindStyleVariations and InitFontList. Mod ver Fx select right font.

Correct, but technically those should be using the en-us names. Since only UI facing names should be localized from what jfkthame tells me.
In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized font name in fontList. Therefore GDI ver choice right font, may be.
Upper layer may need localized font names.
(Assignee)

Comment 143

9 years ago
(In reply to comment #142)
> In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return
> Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized
> font name in fontList. Therefore GDI ver choice right font, may be.
> Upper layer may need localized font names.
Correct, this is the font bug in GDI I already discussed with jfkthame. We just need to figure out why that bug causes correct behavior in your case :).
(In reply to comment #143)
> (In reply to comment #142)
> > In gfxGDIFontList::InitFontList, this func used EnumFontFamiliesExW that return
> > Localized font name(ex. メイリオ(en-us local name as Meiryo)), and store localized
> > font name in fontList. Therefore GDI ver choice right font, may be.
> > Upper layer may need localized font names.
> Correct, this is the font bug in GDI I already discussed with jfkthame. We just
> need to figure out why that bug causes correct behavior in your case :).

I think it's not quite as simple as that (sorry if I gave you a wrong impression when we chatted). As I understand it, we should always be able to recognize the "canonical" (en-us) names, but we should ALSO recognize localized family names when they exist. I know we have code in the Mac implementation to support this. I'd have to re-examine the Windows GDI code to check how (or whether) it's handled there.
Bas, I'm going to do some more testing with your latest patch tomorrow but I did notice one thing while testing today.  You need to test mHasStyles in FindStyleVariations, otherwise you'll populate many times (weee!).

What's the scoop with all the Oblique and Bold Oblique faces?  Those are autogenerated by DirectWrite?  For a font with n weights it appears to be generating n oblique faces, groan.
So the reason the existing code displays the correctly localized fontname is that the default in GDI is to return the name using the default system locale.  DirectWrite always returns the English name, which as it is on OSX is used as a canonical name.

The way to fix this is:

1. Implement the GetFontTable override (gfxWindowsDWriteFontEntry::GetFontTable)
2. Implement the LocalizedName override (gfxWindowsDWriteFontFamily::LocalizedName) using the same logic as in gfxMacFontFamily::LocalizedName, you just need to change [NSFontManager localizedNameForFamily] to the DWrite equivalent.
3. Get rid of mLocalizedName in gfxWindowsDWriteFontFamily and the code associated with it.

Even for users running on CJK systems the large majority of fonts will not have localized font names.  For example, with a default Japanese Windows 7 install there are 196 font families, only 5 of them have Japanese names.  The existing code in gfxPlatformFontList is set up to handle this efficiently with the methods described above.
Also, all four of these patch files have lots of tabs in them, that's a bad thing for the thebes and widget patches, not sure about the build system one.

cat -t xxx.diff | grep '\^I'
The sample code on the docs page for IDWriteFontFamily::GetFamilyNames looks like precisely what you want for getting the family name in the default locale in gfxWindowsDWriteFontFamily::LocalizedName:

IDWriteFontFamily::GetFamilyNames Method
http://msdn.microsoft.com/en-us/library/dd371047(VS.85).aspx
(Assignee)

Comment 149

9 years ago
(In reply to comment #148)
> The sample code on the docs page for IDWriteFontFamily::GetFamilyNames looks
> like precisely what you want for getting the family name in the default locale
> in gfxWindowsDWriteFontFamily::LocalizedName:
> 
> IDWriteFontFamily::GetFamilyNames Method
> http://msdn.microsoft.com/en-us/library/dd371047(VS.85).aspx
This is exacylu what I currently do for mLocalizedName I believe. Only I cache it (i.e. I store it once in FindFontFamilies). I don't use ::GetUserDefaultLocaleName since that is vista and higher only. I could optionally get the function pointer here, but I chose to use Firefox's system locale.
(Assignee)

Comment 150

9 years ago
Created attachment 422775 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v11

Update that should fix the localization font family issues.
Attachment #422132 - Attachment is obsolete: true
Another thing to note here is that DirectWrite is populating font families with "fake" fonts representing the synthetic bold/italic faces.

MS PGothic (family with a single face):
(Regular) face to family (MS PGothic) with style: normal weight: 400 stretch: 0
(Oblique) face to family (MS PGothic) with style: italic weight: 400 stretch: 0
(Bold) face to family (MS PGothic) with style: normal weight: 700 stretch: 0
(Bold Oblique) face to family (MS PGothic) with style: italic weight: 700 stretch: 0
Still wrong.
Font dialog with v11 patch choice wrong font. Same as v10.
Created attachment 422925 [details] [diff] [review]
patch, use GDI interop methods for looking up fullname

+        nsString fullName(mName);
+        nsString faceNameString(faceName.Elements());
+        ToLowerCase(faceNameString);
+        if (!faceNameString.EqualsLiteral("regular") &&
+            !faceNameString.EqualsLiteral("normal")) {
+            fullName.AppendLiteral(" ");
+            fullName.Append(faceName.Elements());
+            // Put this in as the 'standard' face.
+        }

The logic in LookupLocalFont is incorrect, the fullname of the font is a separate data tag in font data from the family name and the face name.  Usually fullname = family + style but there are exceptions, the correct search is for a font for which the fullname in local() matches the fullname in the font data.  This is what the logic in gfxGDIFontList::LookupLocalFont is doing, as noted in comment 78.  I actually don't see a way of pulling out the fullname from individual faces using the DirectWrite API, it appears to only provide access to select fields in the name table.

As I originally suggested, rather than spending time in code that is critical path (FindStyleVariations will be in the startup path if the first page rendered requires font fallback), it makes more sense to use GDI API's in combination with the GDI interop object.  The attached patch is applied on top of thebes v11 and uses the CreateFontFaceFromHdc method to fish out the DirectWrite font.  It also creates a new font entry for this, because the font entry needs to use font properties defined in the @font-face rule (i.e. use the proxy entry settings).  See bug 465463.

I'm no expert on COM+ programming but it looks to me like your code is full of leaks in various places, you're not always calling Release() on objects after using them.  In some cases, this only occurs along error paths.  Getting this right is a complete pain so in my patch I set up a class AutoReleaseIUnknown to deal with auto-cleanup of these objects.  The static function CheckFullname should probably be shared with the GDI code, maybe a static method of gfxWindowsPlatform?
(Assignee)

Comment 154

9 years ago
(In reply to comment #153)
> Created an attachment (id=422925) [details]
> patch, use GDI interop methods for looking up fullname
> 
> +        nsString fullName(mName);
> +        nsString faceNameString(faceName.Elements());
> +        ToLowerCase(faceNameString);
> +        if (!faceNameString.EqualsLiteral("regular") &&
> +            !faceNameString.EqualsLiteral("normal")) {
> +            fullName.AppendLiteral(" ");
> +            fullName.Append(faceName.Elements());
> +            // Put this in as the 'standard' face.
> +        }
> 
> The logic in LookupLocalFont is incorrect, the fullname of the font is a
> separate data tag in font data from the family name and the face name.  Usually
> fullname = family + style but there are exceptions, the correct search is for a
> font for which the fullname in local() matches the fullname in the font data. 
> This is what the logic in gfxGDIFontList::LookupLocalFont is doing, as noted in
> comment 78.  I actually don't see a way of pulling out the fullname from
> individual faces using the DirectWrite API, it appears to only provide access
> to select fields in the name table.
> 
> As I originally suggested, rather than spending time in code that is critical
> path (FindStyleVariations will be in the startup path if the first page
> rendered requires font fallback), it makes more sense to use GDI API's in
> combination with the GDI interop object.  The attached patch is applied on top
> of thebes v11 and uses the CreateFontFaceFromHdc method to fish out the
> DirectWrite font.  It also creates a new font entry for this, because the font
> entry needs to use font properties defined in the @font-face rule (i.e. use the
> proxy entry settings).  See bug 465463.
> 
> I'm no expert on COM+ programming but it looks to me like your code is full of
> leaks in various places, you're not always calling Release() on objects after
> using them.  In some cases, this only occurs along error paths.  Getting this
> right is a complete pain so in my patch I set up a class AutoReleaseIUnknown to
> deal with auto-cleanup of these objects.  The static function CheckFullname
> should probably be shared with the GDI code, maybe a static method of
> gfxWindowsPlatform?
Personally, I still think this is a -very- bad idea, if we could do it with the interop using the LOGFONT it would be alright (but it doesn't seem to work). But this way we let GDI load the font completely. This means we bypass DirectWrite's cross-platform font cache, and we load every font used in this way in GDI. I think performance wise this is a very bad idea. I wanted to do some measurements on its performance but it pretty much doesn't work on my machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 

If the current name is incorrect, I think we should look for a way of fixing this, rather than doing this. Perhaps we could use TryGetFontTable to get the canonical name out of DirectWrite, and use those to populate the hash table. For creating the font entry we should then make the hashtable point to the IDWriteFont rather than the gfxFontEntry, we could then create a new gfxFontEntry surrounding that.

On the COM part/stack based helper, technically we should use the MS CComPtr as the stack based helper to do these things. If we do want to use a stack based helper that is. This also allows all passing byref and such to go right. I'm not sure if we're allowed to pull parts of the ATL into Mozilla code though? If we can't a stack based helper such as AutoReleaseIUnknown would be an option. But we'd probably need to expand it a bit (i.e. it needs to 'forget' a pointer if it's kept around as a class member, or something like that).
(Assignee)

Comment 155

9 years ago
(In reply to comment #153)
> I'm no expert on COM+ programming but it looks to me like your code is full of
> leaks in various places, you're not always calling Release() on objects after
> using them.  In some cases, this only occurs along error paths.
Hrm, I've only been able to find three cases(indeed in error paths). Where else do you see a lot of issues? Not that I'm against using a stack based helper where possible.
(Assignee)

Comment 156

9 years ago
Created attachment 423000 [details] [diff] [review]
Cairo DirectWrite and Direct2D Backends v10

Correct license headers.
Attachment #421562 - Attachment is obsolete: true
Attachment #421562 - Flags: review?(tellrob)
Comment on attachment 421221 [details] [diff] [review]
Widget Direct2D Code v4

Sorry it took so long, especially considering how small this was. One question though, why did you remove that big comment block for ScrollWindowEx?
Attachment #421221 - Flags: review?(jmathies) → review+
Has anybody considered reaching out to Microsoft regarding the missing features in DWrite vs GDI?
I'm seeing asserts and warnings when loading the latest HTML5 spec (my proxy for "big honkin web doc"):

http://dev.w3.org/html5/spec/Overview.html

Asserts/warnings:
0[78b770]: WARNING: Could not get glyph extents (no aContext): file c:/builds/mozcentral/gfx/thebes/src/gfxFont.cpp, line 1250
0[78b770]: ###!!! ASSERTION: illegal height for combined area: 'aCombinedArea.height >= 0', file c:/builds/mozcentral/layout/generic/nsLineBox.cpp, line 515

Lots of the first warning, two of the asserts.  Probably something metrics-related.
Created attachment 423305 [details] [diff] [review]
patch, fixes for local lookup and name table reading

This patch fixes local lookup by using GDI (comment 153) and also fixes the GetFontTable method of gfxDWriteFontList.  I don't think the original version of the GetFontTable method was tested very much, apparently the DirectWrite TryGetFontTable method takes a table tag in reverse endian order compared to GDI's GetFontTable so it was always failing.  Also, the routine was leaking font face objects which caused a failure later on while trying to read in cmap's. With this patch, applied on top of the Thebes v11 patch, the font names for Japanese display correctly.
Attachment #422925 - Attachment is obsolete: true
(In reply to comment #154)

> Personally, I still think this is a -very- bad idea, if we could do it with the
> interop using the LOGFONT it would be alright (but it doesn't seem to work).
> But this way we let GDI load the font completely. This means we bypass
> DirectWrite's cross-platform font cache, and we load every font used in this
> way in GDI. I think performance wise this is a very bad idea. I wanted to do
> some measurements on its performance but it pretty much doesn't work on my
> machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 

Just to be clear, LookupLocalFont is *only* used for looking up local fonts:

body { font-family: Arial, Helevetica, sans-serif; } /* A */

@font-face {
  font-family: Test;
  src: local(Test), url(test.ttf); /* B */
}

@font-face {
  font-family: Test;
  src: local(Test Bold), url(test-bold.ttf); /* B */
}

p { font-family: Test; }

In the style rules above, LookupLocalFont would only be used for cases B, not for general font family lookup (case A).  As such, it's definitely not the common path case by any stretch and only occurs once when the @font-face font is resolved.  Any performance improvement gained by maintaining a fullname hash table has to be weighed against the additional code and memory needed to maintain this.  The code in my patch is basically reusing existing code and assures consistency with the GDI rendering path.  I don't see any need for a hash table unless we can prove that there's a significant performance advantage to maintaining our own hash of font fullnames.

Are you certain that Windows is maintaining a per-app GDI font cache in such a way that using GDI font calls increases our memory footprint?  Have you seen this in practice?
(In reply to comment #155)
> (In reply to comment #153)
> > I'm no expert on COM+ programming but it looks to me like your code is full of
> > leaks in various places, you're not always calling Release() on objects after
> > using them.  In some cases, this only occurs along error paths.
> Hrm, I've only been able to find three cases(indeed in error paths). Where else
> do you see a lot of issues? Not that I'm against using a stack based helper
> where possible.

Here's what I found, although I easily could have missed one ;)

+void
+gfxWindowsDWriteFontFamily::FindStyleVariations()
+{

+        UINT32 length;
+        hr = names->GetStringLength(englishIdx, &length);
+        if (FAILED(hr)) {
+            font->Release();
+            continue;
+        }

Leaks IDWriteLocalizedStrings.

+nsresult
+gfxWindowsDWriteFontEntry::GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8> &aBuffer)

Leaks IDWriteFontFace (both normal and error paths).

+nsresult
+gfxWindowsDWriteFontEntry::ReadCMAP()
+{

+    hr = fontFace->TryGetFontTable(DWRITE_MAKE_OPENTYPE_TAG('c', 'm', 'a', 'p'),
+                                   (const void**)&tableData,
+                                   &len,
+                                   &tableContext,
+                                   &exists);
+    if (FAILED(hr)) {
+        return NS_ERROR_FAILURE;
+    }

Leaks IDWriteFontFace.

+void
+gfxDWriteFontList::InitFontList()
+{

Leaks IDWriteFontCollection (system font collection), all paths.

I think a simple stack-based cleanup class works to simplify code like this.  It's not a silver bullet but it avoids problems as others touch the code later on.

Also, is the code below really kosher?

+class gfxDWriteFontFileLoader : public IDWriteFontFileLoader
+{
+public:
+
+    IFACEMETHOD_(ULONG, AddRef)()
+    {
+        return 1;
+    }
+
+    IFACEMETHOD_(ULONG, Release)()
+    {
+        return 1;
+    }

Again, I'm no COM+ whiz but this makes be nervous...
Created attachment 423315 [details]
startup timings, directwrite vs. gdi

Tryserver build, Thebes v11 + fixup patch
http://tinyurl.com/directd2dtry

I ran some startup timing tests to compare the existing GDI code to the DirectWrite code, using the method described in bug 519445, comment 25.  It basically times the startup + first page load for saved pages from a number of sites.  I used the same build, toggling the 'font.engine.forcegdi' boolean to toggle the two codepaths.

The startup time seems to have improved but in many cases the first page load time is longer.  The sites below are significantly worse with DirectWrite enabled.  I'm guessing system font fallback is occurring and the problem lies along that codepath.

The three numbers are:
  startup (time until onload fires)
  page load (time for an iframe page to load)
  total (sum of the two) 

wikipedia.org dwrite 481 1006 1487
wikipedia.org dwrite 496 1011 1507
wikipedia.org dwrite 497 1009 1506
wikipedia.org dwrite 499 1010 1509
wikipedia.org dwrite 531 980 1511
wikipedia.org dwrite 534 974 1508
wikipedia.org gdi 538 391 929
wikipedia.org gdi 540 414 954
wikipedia.org gdi 542 394 936
wikipedia.org gdi 550 389 939
wikipedia.org gdi 555 415 970
wikipedia.org gdi 556 413 969
www.yahoo.co.jp dwrite 480 938 1418
www.yahoo.co.jp dwrite 487 1042 1529
www.yahoo.co.jp dwrite 491 951 1442
www.yahoo.co.jp dwrite 492 957 1449
www.yahoo.co.jp dwrite 493 928 1421
www.yahoo.co.jp dwrite 496 947 1443
www.yahoo.co.jp gdi 528 277 805
www.yahoo.co.jp gdi 531 278 809
www.yahoo.co.jp gdi 538 276 814
www.yahoo.co.jp gdi 538 278 816
www.yahoo.co.jp gdi 542 275 817
www.yahoo.co.jp gdi 550 281 831
www.yahoo.com dwrite 485 1198 1683
www.yahoo.com dwrite 488 1199 1687
www.yahoo.com dwrite 489 1204 1693
www.yahoo.com dwrite 494 1190 1684
www.yahoo.com dwrite 494 1214 1708
www.yahoo.com dwrite 501 1222 1723
www.yahoo.com gdi 530 483 1013
www.yahoo.com gdi 537 485 1022
www.yahoo.com gdi 537 488 1025
www.yahoo.com gdi 539 525 1064
www.yahoo.com gdi 547 529 1076
www.yahoo.com gdi 552 530 1082
(In reply to comment #159)
> Has anybody considered reaching out to Microsoft regarding the missing features
> in DWrite vs GDI?

I pinged them today about this, we'll see what the response is.
(In reply to comment #164)
> http://tinyurl.com/directd2dtry

Tried it out of curiosity and I'm wondering if he following is known:

1. Fonts look different, as if they were softer/thinner/antialiased more
2. Autoscroll indicator is invisible
3. I am able to break entire browser on one website by zooming in enough - program seems to continue working fine but everything stops rendering (in all windows)
4. Some images (dynamically-added with non-1:1 scaling) are mostly black

For the last two I can easily confirm with new profile and make testcases, if needed.
Looks like the thebes code also needs to be tweaked to assure that WinCE/WinMobile builds work:

e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(147) : error C2653: 'DWriteFactory' : is not a class or namespace name
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(147) : error C3861: 'Instance': identifier not found
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C2653: 'DWriteFactory' : is not a class or namespace name
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C2227: left of '->Release' must point to class/struct/union/generic type
        type is ''unknown-type''
e:/builds/moz2_slave/winmo-hg/build/gfx/thebes/src/gfxWindowsPlatform.cpp(150) : error C3861: 'Instance': identifier not found
(Assignee)

Comment 168

9 years ago
(In reply to comment #162)
> (In reply to comment #154)
> 
> > Personally, I still think this is a -very- bad idea, if we could do it with the
> > interop using the LOGFONT it would be alright (but it doesn't seem to work).
> > But this way we let GDI load the font completely. This means we bypass
> > DirectWrite's cross-platform font cache, and we load every font used in this
> > way in GDI. I think performance wise this is a very bad idea. I wanted to do
> > some measurements on its performance but it pretty much doesn't work on my
> > machine (GetFontData in CheckFullName returns GDI_ERROR on many occasions). 
> 
> Just to be clear, LookupLocalFont is *only* used for looking up local fonts:
> 
> body { font-family: Arial, Helevetica, sans-serif; } /* A */
> 
> @font-face {
>   font-family: Test;
>   src: local(Test), url(test.ttf); /* B */
> }
> 
> @font-face {
>   font-family: Test;
>   src: local(Test Bold), url(test-bold.ttf); /* B */
> }
> 
> p { font-family: Test; }
> 
> In the style rules above, LookupLocalFont would only be used for cases B, not
> for general font family lookup (case A).  As such, it's definitely not the
> common path case by any stretch and only occurs once when the @font-face font
> is resolved.  Any performance improvement gained by maintaining a fullname hash
> table has to be weighed against the additional code and memory needed to
> maintain this.  The code in my patch is basically reusing existing code and
> assures consistency with the GDI rendering path.  I don't see any need for a
> hash table unless we can prove that there's a significant performance advantage
> to maintaining our own hash of font fullnames.
> 
> Are you certain that Windows is maintaining a per-app GDI font cache in such a
> way that using GDI font calls increases our memory footprint?  Have you seen
> this in practice?
Well, I want to test this and analyze it, but as I said, the code isn't working for me? Any ideas?
(Assignee)

Comment 169

9 years ago
(In reply to comment #166)
> (In reply to comment #164)
> > http://tinyurl.com/directd2dtry
> 
> Tried it out of curiosity and I'm wondering if he following is known:
> 
> 1. Fonts look different, as if they were softer/thinner/antialiased more
This is because of directwrite.

> 2. Autoscroll indicator is invisible
Hrm, I can reproduce this, odd.

> 3. I am able to break entire browser on one website by zooming in enough -
> program seems to continue working fine but everything stops rendering (in all
> windows)
> 4. Some images (dynamically-added with non-1:1 scaling) are mostly black
> 
> For the last two I can easily confirm with new profile and make testcases, if
> needed.
That would be really nice :-).
(Assignee)

Comment 170

9 years ago
(In reply to comment #164)
> Created an attachment (id=423315) [details]
> startup timings, directwrite vs. gdi
> 
> Tryserver build, Thebes v11 + fixup patch
> http://tinyurl.com/directd2dtry
> 
> I ran some startup timing tests to compare the existing GDI code to the
> DirectWrite code, using the method described in bug 519445, comment 25.  It
> basically times the startup + first page load for saved pages from a number of
> sites.  I used the same build, toggling the 'font.engine.forcegdi' boolean to
> toggle the two codepaths.
> 
> The startup time seems to have improved but in many cases the first page load
> time is longer.  The sites below are significantly worse with DirectWrite
> enabled.  I'm guessing system font fallback is occurring and the problem lies
> along that codepath.

Although this could be the case, there's two other things to consider: Direct2D could be slower for some reason or another. The entire page rendering would be different. The second option is that DirectWrite simply does significantly more than GDI, sub-pixel positioning and vertical anti-aliasing would cause an increase in work.
(Assignee)

Comment 171

9 years ago
(In reply to comment #163)
> (In reply to comment #155)
> > (In reply to comment #153)
> Here's what I found, although I easily could have missed one ;)
> 
> +void
> +gfxWindowsDWriteFontFamily::FindStyleVariations()
> +{
> 
> +        UINT32 length;
> +        hr = names->GetStringLength(englishIdx, &length);
> +        if (FAILED(hr)) {
> +            font->Release();
> +            continue;
> +        }
> 
> Leaks IDWriteLocalizedStrings.
> 
> +nsresult
> +gfxWindowsDWriteFontEntry::GetFontTable(PRUint32 aTableTag, nsTArray<PRUint8>
> &aBuffer)
> 
> Leaks IDWriteFontFace (both normal and error paths).
> 
> 
> +void
> +gfxDWriteFontList::InitFontList()
> +{
> 
> Leaks IDWriteFontCollection (system font collection), all paths.
> 
Ah, I found a couple of others, yes, but I missed these two, thanks!

> I think a simple stack-based cleanup class works to simplify code like this. 
> It's not a silver bullet but it avoids problems as others touch the code later
> on.
> 
Yes, I agree. Although now that I think about it, doesn't one of Mozilla's auto pointers also work on IUnknowns on windows? I seem to recall seeing code for this.

> Also, is the code below really kosher?
> 
> +class gfxDWriteFontFileLoader : public IDWriteFontFileLoader
> +{
> +public:
> +
> +    IFACEMETHOD_(ULONG, AddRef)()
> +    {
> +        return 1;
> +    }
> +
> +    IFACEMETHOD_(ULONG, Release)()
> +    {
> +        return 1;
> +    }
> 
> Again, I'm no COM+ whiz but this makes be nervous...
This should be fine. A problem could occur if someone tries to release until it returns 0, but that's very bad practice anyway, and I've only ever done that when debugging :-). Other than that the classes comes and goes on the stack, I know of several MS examples that do this too. Perhaps Robarnold can confirm, as he's more knowledgeable on COM than I am.
Created attachment 423337 [details]
crashes

(In reply to comment #169)
> That would be really nice :-).

I tried to minimise the "stops rendering" testcase and got a crashing testcase instead. This website consists of just one character - long dash. I'm guessing I could just paste it to a comment but I won't be evil :)

Black-images testcase next.
(In reply to comment #172)
> I tried to minimise the "stops rendering" testcase and got a crashing testcase
> instead. This website consists of just one character - long dash. I'm guessing
> I could just paste it to a comment but I won't be evil :)

Although this page claims to be UTF-8, it appears that it is in fact encoded as Latin-1 (ISO 8859-1 or Windows codepage 1252). The intended "long dash", when interpreted as UTF-8, is therefore an encoding error and should be replaced with U+FFFD.

So I'd guess the failure is caused by the presence of U+FFFD in the text to be rendered. Of course, this shouldn't cause a crash, but it is likely to trigger system font fallback (at least), and may not be available in any font (in which case it should fall back to hexbox rendering). Perhaps one of these paths is broken.
(In reply to comment #151)
> Another thing to note here is that DirectWrite is populating font families with
> "fake" fonts representing the synthetic bold/italic faces.
> 
> MS PGothic (family with a single face):
> (Regular) face to family (MS PGothic) with style: normal weight: 400 stretch: 0
> (Oblique) face to family (MS PGothic) with style: italic weight: 400 stretch: 0
> (Bold) face to family (MS PGothic) with style: normal weight: 700 stretch: 0
> (Bold Oblique) face to family (MS PGothic) with style: italic weight: 700
> stretch: 0

If it's at all feasible, I'd prefer that we don't create such "fake font entries"; the font family should contain only the real faces that are actually available, and any synthetic styling needed should be handled at the point where an actual Cairo font is instantiated from one of the available faces.

Creating fake font entries causes us to waste time and space reading the same cmap multiple times. This is particularly unfortunate considering that many of the fonts that lack a "full" set of (at least) the 4 standard styles are large-repertoire CJK fonts.
Created attachment 423338 [details]
image adding b0rk

dynamically adds images. They don't decode well, not incrementally.
Uncomment the width change to make their b0rk black, otherwise it's transparent.
(hopefully adding http images works from https?)
(Assignee)

Comment 176

9 years ago
(In reply to comment #175)
> Created an attachment (id=423338) [details]
> image adding b0rk
> 
> dynamically adds images. They don't decode well, not incrementally.
> Uncomment the width change to make their b0rk black, otherwise it's
> transparent.
> (hopefully adding http images works from https?)

This is due to bug 534787. I have a fix for this on my tree, and a proper fix will be landed at some point.
(In reply to comment #171)
> This should be fine. A problem could occur if someone tries to release until it
> returns 0, but that's very bad practice anyway, and I've only ever done that
> when debugging :-). Other than that the classes comes and goes on the stack, I
> know of several MS examples that do this too. Perhaps Robarnold can confirm, as
> he's more knowledgeable on COM than I am.
You can use nsRefPtr with COM objects.
(Assignee)

Comment 178

9 years ago
Created attachment 423408 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v12

Fix the crash on invalid characters. Also fix windows mobile build, and use nsRefPtr in main thebes code.
Attachment #422775 - Attachment is obsolete: true

Updated

9 years ago
Depends on: 542162
(Assignee)

Comment 179

9 years ago
Created attachment 424484 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v13

New patch for thebes. This uses the new code on trunk for fullname lookup, and it also fixed a bunch of glyph cluster issues. This now passes all the ref tests that I've been able to find for fonts. Except for a couple of mixed-charset bidi issues (where the non-mixed version used kerning, but the mixed-charset version does not, according to jfkthame on XP neither used kerning, which is why the reftests passed. On my windows 7 machine, they also fail when using GDI.)
Attachment #423408 - Attachment is obsolete: true
Attachment #424484 - Flags: review?(jfkthame)
Attachment #424484 - Flags: review?(jdaggett)
Created attachment 424541 [details]
screenshot, dwrite vs. gdi subpixel spacing

Comparison of Calluna (OpenType PS font) rendered with DWrite (v13 patch) and with GDI, rendered at 8x magnification.  Best viewed sitting several meters away from your monitor.

http://people.mozilla.org/~jdaggett/tests/subpixelspacing.html

Note how the stem of the initial T slides from one pixel to the next in the DWrite case.  With GDI is "jumps" at 0.5 pixels of shift.  Larger sizes of TrueType fonts also display without the jaggies present in the GDI case (yay!).
(In reply to comment #179)
> Created an attachment (id=424484) [details]
> Thebes DirectWrite and Direct2D Code v13

General cleanup:

Several of the files in this patch still contain tab characters.  Those need to be cleaned out.  Also, there are lots of places where the 80-character per line rule is ignored.  We already have gfx code that ignores this in different places but that's a bug, we shouldn't be creating new code that does this.

+++ b/gfx/thebes/public/gfxWindowsDWriteFonts.h
+++ b/gfx/thebes/src/gfxWindowsDWriteFonts.cpp
+++ b/gfx/thebes/public/gfxWindowsGDIFonts.h
+++ b/gfx/thebes/src/gfxWindowsGDIFonts.cpp

As Jonathan noted in comment 112, I think it would be *much* simpler to skip changing gfxWindowsFonts to gfxWindowsGDIFonts and trim the 'Windows' from the DWrite files.  This code will go through another thrash as part of Jonathan's harfbuzz work, we can figure out the right name changes at that point.

+++ b/gfx/thebes/src/gfxDWriteCommon.h
+++ b/gfx/thebes/src/gfxDWriteCustomFonts.cpp
+++ b/gfx/thebes/src/gfxDWriteCustomFonts.h

I don't see the need for these to be separate files, I think it would be better to keep them all in gfxDWriteFonts.h/cpp.

+class DWriteFactory
+{
+public:
+    static IDWriteFactory *Instance()

So this class has a single static method and a couple statics associated with it?  Seems like this belongs in gfxWindowsPlatform instead.

+    static IDWriteFactory *mInstance;

Use nsRefPtr<IDWriteFactory> instead and skip the release.

+                rv = prefSvc->GetBranch("font.engine.", getter_AddRefs(prefBranch));
+                if (NS_SUCCEEDED(rv)) {
+                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
+                    if (NS_FAILED(rv)) {
+                        forceGDI = PR_FALSE;
+                    }

Rather than 'font.engine.forcegdi', use 'gfx.font_rendering.directwrite.enabled' and add an entry to modules/libpref/src/init/all.js in the Windows-only section.  We'll need to add other conditionals like this for forcing Cleartype-rendering on XP, I'd like to keep those grouped together.  For initial checkin we should leave this set to 'false' and we can flip it to 'true' once it's had a bit of time to get banged on.

+IDWriteFontFileLoader* gfxDWriteFontFileLoader::mInstance = NULL;

Use nsRefPtr<IDWriteFontFileLoader>.

+        gfxWindowsDWriteFontEntry *fe = 
+            new gfxWindowsDWriteFontEntry(fullName, font);
+        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
+                      font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
+        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
+        PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
+        	
+        weight = NS_MAX<PRUint16>(100, weight);
+        weight = NS_MIN<PRUint16>(900, weight);
+	
+        fe->mWeight = weight;

Code like this appears several times.  Make the weight, style, stretch part of the constructor.

+gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData, PRUint32 aLength)
+{
+    mData = new PRUint8[aLength];
+    mDataSize = aLength;
+    memcpy(mData, aData, aLength);
+}

Why is this copy needed?  Seems like could rework the code in MakePlatformFont to avoid this copy.

+    gfxWindowsDWriteFontEntry(const nsAString& aFaceName,
+                              IDWriteFont *aFont, 
+                              IDWriteFontFile *aFontFile = NULL) 
+      : gfxFontEntry(aFaceName), mFont(aFont), mFontFile(aFontFile)
+    {
+    }

Make two constructors, one with a IDWriteFont, the other with a IDWriteFontFile.

+void
+gfxWindowsDWriteFontFamily::FindStyleVariations()

There's a lot of code here to get the fullname for each face, I don't think that's needed at all, this is only used by the font cache and never for OS lookups (correct me here if I missed something).  Seems like just mName + i would work just as well for example.

+        if (exists) {
+            UINT32 eng;
+            unspec->FindLocaleName(L"en-us", &eng, &exists);
+            WCHAR test[256];
+            if (exists) {
+                unspec->GetString(eng, test, 256);
+            }
+            int a = 1;
+        }

Delete debugging (?) code.

+    /**
+     * Second pass, for all oblique entries we search for an identical italic
+     * entry. If one exists we remove the oblique entry from the available
+     * fonts list, since gfxFontEntry can't make the distinction.
+     *
+     * TODO: Current algorithm is not the most efficient but it does the job
+     * with little code complexity. Need to consider if something more advanced
+     * is required.
+     */
+    for (unsigned int i = 0; i < obliqueEntries.Length(); i++) {
+        for (unsigned int c = 0; c < mAvailableFonts.Length(); c++) {
+            if (mAvailableFonts[c].get() != obliqueEntries[i] &&
+                mAvailableFonts[c]->mWeight == obliqueEntries[i]->mWeight &&
+                mAvailableFonts[c]->mItalic == PR_TRUE &&
+                mAvailableFonts[c]->mStretch == obliqueEntries[i]->mStretch) {
+                    mAvailableFonts.RemoveElement(obliqueEntries[i]);
+                    break;
+            }
+        }
+    }

Ack.  I think we can avoid this by simply skipping faces that have a "simulation" (i.e. fake bold/oblique) set based on font->GetSimulations().  My guess is that Windows 7 does not ship with any "real" oblique fonts.

+PRBool
+gfxWindowsDWriteFontEntry::SupportsLangGroup(const nsACString &aLangGroup) const
+{
+    // TODO: Implement me.
+    return PR_TRUE;
+}
+
+PRBool
+gfxWindowsDWriteFontEntry::MatchesGenericFamily(const nsACString &aGeneric) const
+{
+    // TODO: Implement me.
+    return PR_TRUE;
+}
+
+PRBool
+gfxWindowsDWriteFontEntry::SupportsRange(PRUint8 aRangeBit)
+{
+    // TODO: Implement me!!
+    return PR_TRUE;
+}

Implementation needed.

+    if (mFont) {
+        hr = mFont->CreateFontFace(getter_AddRefs(fontFace));
+    } else if (mFontFile) {
+        IDWriteFontFile *fontFile = mFontFile.get();
+        hr = DWriteFactory::Instance()->CreateFontFace(
+            mFaceType,
+            1,
+            &fontFile,
+            0,
+            DWRITE_FONT_SIMULATIONS_NONE,
+            getter_AddRefs(fontFace));
+    }

Create a private helper method 'GetFontFace' so that you're not repeating code in multiple places.

+gfxFontEntry *
+gfxDWriteFontList::GetDefaultFont(const gfxFontStyle *aStyle,
+                                  PRBool &aNeedsBold)
+{
+    HRESULT hr;
+    nsRefPtr<IDWriteFontCollection> systemFonts;
+    hr = DWriteFactory::Instance()->GetSystemFontCollection(getter_AddRefs(systemFonts));
+    NS_ASSERTION(SUCCEEDED(hr), "GetSystemFontCollection failed!");
+    UINT32 idx = 0;
+    BOOL exists;
+    hr = systemFonts->FindFamilyName(L"tahoma", &idx, &exists);

This code needs to match the logic in gfxGDIFontList::GetDefaultFont, the default font varies based on the OS locale, just setting this to Tahoma is wrong.

+class gfxWindowsDWriteFontFamily : public gfxFontFamily
+{
+public:
+    /**
+     * Constructs a new DWriteFont Family.
+     *
+     * \param aName Name identifying the family
+     * \param aFamily IDWriteFontFamily object representing the directwrite
+     * family object.
+     */
+    gfxWindowsDWriteFontFamily(const nsAString& aName, 
+                               IDWriteFontFamily *aFamily,
+                               const nsAString& aLocalName) :
+        gfxFontFamily(aName), mDWFamily(aFamily),
+        mLocalizedName(aLocalName) { }
+    virtual ~gfxWindowsDWriteFontFamily();
+    
+    virtual void FindStyleVariations();
+
+    virtual void LocalizedName(nsAString& aLocalizedName); 
+protected:
+    /** This font family's directwrite fontfamily object */
+    nsRefPtr<IDWriteFontFamily> mDWFamily;
+    nsString mLocalizedName;
+};

We don't need to store the localized family, this is *only* used when the user opens the content prefs panel.  Better to look this up within LocalizedName() rather than spend time in gfxDWriteFontList::InitFontList which affects startup time.

+void
+gfxPlatformFontList::EnsureStylesLoaded()
+{
+    // Prevent freezing the main thread if mIncrement somehow became 0.
+    if (mStartIndex < mNumFamilies && mIncrement > 0) {
+        while (!RunLoader()) {}
+        CancelLoader();
+    }
+}
+

This isn't called anywhere, trim.

Jeff or Joe should probably review the gfxD2DSurface code.
Comment on attachment 424484 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v13

Please grep your files/patches for TAB characters (again), as it looks like a few have crept back in during editing.

> +        nsRefPtr<IDWriteLocalizedStrings> unspec;
> +        BOOL exists;
> +        hr = font->GetInformationalStrings(DWRITE_INFORMATIONAL_STRING_WIN32_SUBFAMILY_NAMES,
> +                                           getter_AddRefs(unspec),
> +                                           &exists);
> +        if (exists) {
> +            UINT32 eng;
> +            unspec->FindLocaleName(L"en-us", &eng, &exists);
> +            WCHAR test[256];
> +            if (exists) {
> +                unspec->GetString(eng, test, 256);
> +            }
> +            int a = 1;
> +        }

This looks like leftover debugging/investigative code!

> +        nsString fullName(mName);
> +        nsString faceNameString(faceName.Elements());
> +        ToLowerCase(faceNameString);
> +        if (!faceNameString.EqualsLiteral("regular") &&
> +            !faceNameString.EqualsLiteral("normal")) {

I don't think "normal" should be excluded here, the "rule" (or rather recommendation) only applies to the specific name "regular"...

> +            fullName.AppendLiteral(" ");
> +            fullName.Append(faceName.Elements());
> +            // Otherwise put this is the 'standard' face.
> +        }

...but do we still need to do this at all, with the new fullname-reading code available? I don't like the idea that we could end up getting two different versions of the "full name" for a face, one by concatenating family + style (here) and another later by reading the actual name table.


> +    PRUint8 *tableData;
> +    PRUint32 len;
> +    void *tableContext = NULL;
> +    BOOL exists;
> +    hr = fontFace->TryGetFontTable(NS_SWAP32(aTableTag),
> +                                   (const void**)&tableData,
> +                                   &len,
> +                                   &tableContext,
> +                                   &exists);
> +
> +    if (FAILED(hr) || !exists) {
> +        return NS_ERROR_FAILURE;
> +    }
> +    aBuffer.SetLength(len);

nsTArray<T>::SetLength returns a boolean indicating success/failure; should check it (here and elsewhere), and bail with NS_ERROR_OUT_OF_MEMORY if it fails.


> +    if (exists) {
> +        rv = gfxFontUtils::ReadCMAP(tableData,
> +                                    len,
> +                                    mCharacterMap,
> +                                    isUnicode,
> +                                    isSymbol);
> +    }
> +
> +    if (tableContext) {
> +        fontFace->ReleaseFontTable(tableContext);
> +    }
> +
> +    mCmapInitialized = PR_TRUE;
> +    return NS_OK;

Shouldn't this return rv (the result from gfxFontUtils::ReadCMAP), rather than just NS_OK? Although the table data was found, interpreting it may have failed.


> +        famName.SetLength(length + 1);

> +        famLocalName.SetLength(length + 1);

Check for failures.


> +/**
> + * gfxWindowsDWFontFamily is a class that describes one of the fonts on the
> + * users system.  It holds each gfxWindowsDWFontEntry (maps more directly to
> + * a font face) which holds font type, charset info and character map info.
> + */

Class names in the comment are out of date - the actual code has "DWrite", not "DW".


> +    void IndexFullNames();

This is obsolete, I believe.


> +void TextAnalysis::SplitCurrentRun(UINT32 splitPosition)
> +{
> +    if (!mCurrentRun) {
> +        // Shouldn't be calling this when no current run is set!

In that case, I'd make it an assertion rather than just a comment. If someone breaks the logic, we should tell them.


> +gfxWindowsDWriteFont::~gfxWindowsDWriteFont()
> +{
> +    if (mCairoFontFace) {
> +        cairo_font_face_destroy(mCairoFontFace);
> +    }
> +    if (mCairoScaledFont) {
> +        cairo_scaled_font_destroy(mCairoScaledFont);
> +    }
> +}

Looks like this will leak the font's mMetrics, I think.


> +    UINT32 ucs = L'0';
> +    mFontFace->GetGlyphIndicesA(&ucs, 1, &glyph);
> +    mFontFace->GetDesignGlyphMetrics(&glyph, 1, &metrics);

What happens if there's no "0" character in the font? Will GetGlyphIndicesA fail? Should be checking the result code.

> +    mMetrics->zeroOrAveCharWidth = ((gfxFloat)metrics.advanceWidth / fontMetrics.designUnitsPerEm) * mStyle.size;
> +    ucs = L'x';
> +    mFontFace->GetGlyphIndicesA(&ucs, 1, &glyph);
> +    mFontFace->GetDesignGlyphMetrics(&glyph, 1, &metrics);    

Again, what if there's no "x"? Also, trailing whitespace here.


> +cairo_font_face_t *
> +gfxWindowsDWriteFont::CairoFontFace()
> +{
> +    if (!mCairoFontFace) {
> +#ifdef CAIRO_HAS_DWRITE_FONT
> +        mCairoFontFace = 
> +            cairo_dwrite_font_face_create_for_dwrite_fontface(
> +            ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> +#endif
> +    }
> +    return mCairoFontFace;
> +}

What would happen if the #ifdef here is false? Seems like that could be a problem - should there be a configure-time dependency or something so that we don't get into that situation?


> +        UINT32 maxGlyphs = 3 * range.Length() / 2 + 16;
> +        nsAutoTArray<UINT16, 400> clusters;
> +        nsAutoTArray<UINT16, 400> indices;
> +        clusters.SetLength(range.Length());
> +        indices.SetLength(maxGlyphs);
> +        nsAutoTArray<DWRITE_SHAPING_TEXT_PROPERTIES, 400> textProperties;
> +        nsAutoTArray<DWRITE_SHAPING_GLYPH_PROPERTIES, 400> glyphProperties;
> +        textProperties.SetLength(maxGlyphs);
> +        glyphProperties.SetLength(maxGlyphs);

Need to check the SetLength calls for failure.

> +        advances.SetLength(actualGlyphs);
> +        glyphOffsets.SetLength(actualGlyphs);

Ditto.
(Assignee)

Comment 183

9 years ago
I've replied in the places where I have questions :).

(In reply to comment #181)
> (In reply to comment #179)
> 
> +++ b/gfx/thebes/src/gfxDWriteCommon.h
> +++ b/gfx/thebes/src/gfxDWriteCustomFonts.cpp
> +++ b/gfx/thebes/src/gfxDWriteCustomFonts.h
> 
> I don't see the need for these to be separate files, I think it would be better
> to keep them all in gfxDWriteFonts.h/cpp.
Personally I think this is a bad idea. There's a lot of places in our tree where I feel the large amount of code/classes in a single file is already compromising readability and making the code harder to understand. I wouldn't mind merging some of these files to less files, but I'm not sure how to merge them in that case :). I could put all the stuff from CustomFonts.h in Common.h and create a common.cpp, clearing out one file. Of course if you guys feel I should just stuff all this in the main files, I can do that.
> 
> +class DWriteFactory
> +{
> +public:
> +    static IDWriteFactory *Instance()
> 
> So this class has a single static method and a couple statics associated with
> it?  Seems like this belongs in gfxWindowsPlatform instead.
> 
> +    static IDWriteFactory *mInstance;
> 
> Use nsRefPtr<IDWriteFactory> instead and skip the release.
> 
> +                rv = prefSvc->GetBranch("font.engine.",
> getter_AddRefs(prefBranch));
> +                if (NS_SUCCEEDED(rv)) {
> +                    rv = prefBranch->GetBoolPref("forcegdi", &forceGDI);
> +                    if (NS_FAILED(rv)) {
> +                        forceGDI = PR_FALSE;
> +                    }
> 
> Rather than 'font.engine.forcegdi', use
> 'gfx.font_rendering.directwrite.enabled' and add an entry to
> modules/libpref/src/init/all.js in the Windows-only section.  We'll need to add
> other conditionals like this for forcing Cleartype-rendering on XP, I'd like to
> keep those grouped together.  For initial checkin we should leave this set to
> 'false' and we can flip it to 'true' once it's had a bit of time to get banged
> on.

This will be done in the final iteration where I also remove using Direct2D by default.
> 
> +IDWriteFontFileLoader* gfxDWriteFontFileLoader::mInstance = NULL;
> 
> Use nsRefPtr<IDWriteFontFileLoader>.
> 
> +        gfxWindowsDWriteFontEntry *fe = 
> +            new gfxWindowsDWriteFontEntry(fullName, font);
> +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
> +                      font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
> +        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
> +        PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
> +            
> +        weight = NS_MAX<PRUint16>(100, weight);
> +        weight = NS_MIN<PRUint16>(900, weight);
> +    
> +        fe->mWeight = weight;
> 
> Code like this appears several times.  Make the weight, style, stretch part of
> the constructor.
Shouldn't we make this constructor arguments on the baseclass then? Every implementation I've looked at seems to either add code for this in the constructor, or do what I did. Seems to me like we'd be better off adding this to the initializer list on the base class.

> 
> +gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData, PRUint32
> aLength)
> +{
> +    mData = new PRUint8[aLength];
> +    mDataSize = aLength;
> +    memcpy(mData, aData, aLength);
> +}
> 
> Why is this copy needed?  Seems like could rework the code in MakePlatformFont
> to avoid this copy.
Well, I need to change ownership of the data. A change as you suggest would require a big warning sign though. Since this would mean I need to use NS_Free in the destructor. It means -ONLY- an array allocated with NS_Alloc can be passed in here. This is a dangerous situation which is why I copied it. The RAM copy has negligible overhead I believe.

> 
> +void
> +gfxWindowsDWriteFontFamily::FindStyleVariations()
> 
> There's a lot of code here to get the fullname for each face, I don't think
> that's needed at all, this is only used by the font cache and never for OS
> lookups (correct me here if I missed something).  Seems like just mName + i
> would work just as well for example.
At the moment, you're correct (I know this because I did what you're suggested at some point :)). However I think for debugging/maintenance purposes this is a bad idea. IF we do this it should be renamed Identifier() or something like that. We can't go calling it 'name' and then not actually have it contain the name. That would make for horrible API behaviour.
> +    /**
> +     * Second pass, for all oblique entries we search for an identical italic
> +     * entry. If one exists we remove the oblique entry from the available
> +     * fonts list, since gfxFontEntry can't make the distinction.
> +     *
> +     * TODO: Current algorithm is not the most efficient but it does the job
> +     * with little code complexity. Need to consider if something more
> advanced
> +     * is required.
> +     */
> +    for (unsigned int i = 0; i < obliqueEntries.Length(); i++) {
> +        for (unsigned int c = 0; c < mAvailableFonts.Length(); c++) {
> +            if (mAvailableFonts[c].get() != obliqueEntries[i] &&
> +                mAvailableFonts[c]->mWeight == obliqueEntries[i]->mWeight &&
> +                mAvailableFonts[c]->mItalic == PR_TRUE &&
> +                mAvailableFonts[c]->mStretch == obliqueEntries[i]->mStretch) {
> +                    mAvailableFonts.RemoveElement(obliqueEntries[i]);
> +                    break;
> +            }
> +        }
> +    }
> 
> Ack.  I think we can avoid this by simply skipping faces that have a
> "simulation" (i.e. fake bold/oblique) set based on font->GetSimulations().  My
> guess is that Windows 7 does not ship with any "real" oblique fonts.
True, we can, GetSimulations() indicates it's a simulation for most oblique fonts. Despite that, however, I felt they should still be listed if there's no italic font available? If this is incorrect ofcourse, it's -much- easier to just take out all Oblique simulations.
> 
> Implementation needed.

Yeah... can't find any DirectWrite code to do any of these things at this point though. Most of them can be solved, MatchesGenericFamily I'm not so sure though. DirectWrite seems to have no concept of generic families.

> This code needs to match the logic in gfxGDIFontList::GetDefaultFont, the
> default font varies based on the OS locale, just setting this to Tahoma is
> wrong.

Yep, I realize that. I can't seem to find anything to do this in DirectWrite though, I will look some more.

> 
> We don't need to store the localized family, this is *only* used when the user
> opens the content prefs panel.  Better to look this up within LocalizedName()
> rather than spend time in gfxDWriteFontList::InitFontList which affects startup
> time.

We basically already have it. Since we've already gotten the localized strings set. The time spent in the lookup of the localized name is extremely small (we've already gotten the IDWriteLocalizedStrings set, so the table's already been grabbed). Do you really think it's worth doing this lookup in LocalizedName(), I realise this is not used a lot at the moment, but for future API robustness I'd say it's wiser to store this.
(Assignee)

Comment 184

9 years ago
(In reply to comment #182)
> (From update of attachment 424484 [details] [diff] [review])
> > +cairo_font_face_t *
> > +gfxWindowsDWriteFont::CairoFontFace()
> > +{
> > +    if (!mCairoFontFace) {
> > +#ifdef CAIRO_HAS_DWRITE_FONT
> > +        mCairoFontFace = 
> > +            cairo_dwrite_font_face_create_for_dwrite_fontface(
> > +            ((gfxWindowsDWriteFontEntry*)mFontEntry.get())->mFont, mFontFace);
> > +#endif
> > +    }
> > +    return mCairoFontFace;
> > +}
> 
> What would happen if the #ifdef here is false? Seems like that could be a
> problem - should there be a configure-time dependency or something so that we
> don't get into that situation?

It should be unreachable code. Since DWriteFactory would never construct then. And therefor the gfxWindowsPlatform code would never create a DWriteFontList. I realize this isn't too pretty :(.
(In reply to comment #182)
> (From update of attachment 424484 [details] [diff] [review])
> Please grep your files/patches for TAB characters (again), as it looks like a
> few have crept back in during editing.

Bas, which editor are you using? All the majors have options to expand tab as keystroke into the right number of spaces.

/be
(In reply to comment #183)
> > +    mData = new PRUint8[aLength];
> > +    mDataSize = aLength;
> > +    memcpy(mData, aData, aLength);
If you do keep this, you should probably use nsMemory::Clone here which gives you error checking on new for free (which you should also be doing).
(Assignee)

Comment 187

9 years ago
(In reply to comment #185)
> (In reply to comment #182)
> > (From update of attachment 424484 [details] [diff] [review] [details])
> > Please grep your files/patches for TAB characters (again), as it looks like a
> > few have crept back in during editing.
> 
> Bas, which editor are you using? All the majors have options to expand tab as
> keystroke into the right number of spaces.
> 
> /be

Visual Studio, it's something obscure like Ctrl+F and then Ctrl+<something>. And you have to do it on a per file basis I think. Then again, Visual Studio is extremely unintuitive in some things so I might've missed a feature.
(In reply to comment #183)

> > I don't see the need for these to be separate files, I think it would be better
> > to keep them all in gfxDWriteFonts.h/cpp.
> Personally I think this is a bad idea. There's a lot of places in our tree
> where I feel the large amount of code/classes in a single file is already
> compromising readability and making the code harder to understand.

I'm inclined to agree; I'd prefer to split rather than lump things together. (One of these days I'd love to split various classes out from gfxFont.h/cpp, for example.)

> > +class DWriteFactory
> > +{
> > +public:
> > +    static IDWriteFactory *Instance()
> > 
> > So this class has a single static method and a couple statics associated with
> > it?  Seems like this belongs in gfxWindowsPlatform instead.

It would be #ifdef'd within gfxWindowsPlatform, wouldn't it? (Presumably mobile builds don't have this available?) In that case, making it separate seems tidier to me.


> > +        gfxWindowsDWriteFontEntry *fe = 
> > +            new gfxWindowsDWriteFontEntry(fullName, font);
> > +        fe->mItalic = (font->GetStyle() == DWRITE_FONT_STYLE_ITALIC ||
> > +                      font->GetStyle() == DWRITE_FONT_STYLE_OBLIQUE);
> > +        fe->mStretch = FontStretchFromDWriteStretch(font->GetStretch());
> > +        PRUint16 weight = PR_ROUNDUP(font->GetWeight() - 50, 100);
> > +            
> > +        weight = NS_MAX<PRUint16>(100, weight);
> > +        weight = NS_MIN<PRUint16>(900, weight);
> > +    
> > +        fe->mWeight = weight;
> > 
> > Code like this appears several times.  Make the weight, style, stretch part of
> > the constructor.
> Shouldn't we make this constructor arguments on the baseclass then? Every
> implementation I've looked at seems to either add code for this in the
> constructor, or do what I did. Seems to me like we'd be better off adding this
> to the initializer list on the base class.

As these are all being obtained from the font, and the font is passed to the constructor, it seems to me that the most logical thing is for the constructor to do this internally.


> > +gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData, PRUint32
> > aLength)
> > +{
> > +    mData = new PRUint8[aLength];
> > +    mDataSize = aLength;
> > +    memcpy(mData, aData, aLength);
> > +}
> > 
> > Why is this copy needed?  Seems like could rework the code in MakePlatformFont
> > to avoid this copy.
> Well, I need to change ownership of the data. A change as you suggest would
> require a big warning sign though. Since this would mean I need to use NS_Free
> in the destructor. It means -ONLY- an array allocated with NS_Alloc can be
> passed in here. This is a dangerous situation which is why I copied it. The RAM
> copy has negligible overhead I believe.

I think the idea would be that gfxDWriteFontFileStream doesn't need to know anything about the ownership or destruction of the data. MakePlatformFont is already making its own copy (due to the rename operation), and that should persist long enough for the load to be completed.

> > 
> > +void
> > +gfxWindowsDWriteFontFamily::FindStyleVariations()
> > 
> > There's a lot of code here to get the fullname for each face, I don't think
> > that's needed at all, this is only used by the font cache and never for OS
> > lookups (correct me here if I missed something).  Seems like just mName + i
> > would work just as well for example.
> At the moment, you're correct (I know this because I did what you're suggested
> at some point :)). However I think for debugging/maintenance purposes this is a
> bad idea. IF we do this it should be renamed Identifier() or something like
> that. We can't go calling it 'name' and then not actually have it contain the
> name. That would make for horrible API behaviour.

So if it's just an identifier for gfxFontCache, you can use family + face with no need to lowercase, no need to check for "Regular". And yes, we should call it Identifier and not confuse it with the actual fullname (that we're reading and indexing separately).

> True, we can, GetSimulations() indicates it's a simulation for most oblique
> fonts. Despite that, however, I felt they should still be listed if there's no
> italic font available? If this is incorrect ofcourse, it's -much- easier to
> just take out all Oblique simulations.

If there's no true oblique, and no italic, then no need to create a font entry at all; what we do elsewhere in that case is to synthesize an oblique face on demand (with the cairo font matrix).

However, we can't ignore the possibility of "true" oblique faces - I think it's very likely that some of the sans or monospace fonts will include oblique faces. (Isn't that already the case for things like Courier New?)

> > We don't need to store the localized family, this is *only* used when the user
> > opens the content prefs panel.  Better to look this up within LocalizedName()
> > rather than spend time in gfxDWriteFontList::InitFontList which affects startup
> > time.
> 
> We basically already have it. Since we've already gotten the localized strings
> set. The time spent in the lookup of the localized name is extremely small
> (we've already gotten the IDWriteLocalizedStrings set, so the table's already
> been grabbed). Do you really think it's worth doing this lookup in
> LocalizedName(), I realise this is not used a lot at the moment, but for future
> API robustness I'd say it's wiser to store this.

Hmmm. I don't have a strong opinion one way or the other on this one.
(Assignee)

Comment 189

9 years ago
(In reply to comment #188)
> (In reply to comment #183)
>As these are all being obtained from the font, and the font is passed to the
>constructor, it seems to me that the most logical thing is for the constructor
>to do this internally.
Not true. In @font-face cases they're not determined by the actual font.

> > > Why is this copy needed?  Seems like could rework the code in MakePlatformFont
> > > to avoid this copy.
> > Well, I need to change ownership of the data. A change as you suggest would
> > require a big warning sign though. Since this would mean I need to use NS_Free
> > in the destructor. It means -ONLY- an array allocated with NS_Alloc can be
> > passed in here. This is a dangerous situation which is why I copied it. The RAM
> > copy has negligible overhead I believe.
> 
> I think the idea would be that gfxDWriteFontFileStream doesn't need to know
> anything about the ownership or destruction of the data. MakePlatformFont is
> already making its own copy (due to the rename operation), and that should
> persist long enough for the load to be completed.
The FontFileReference can hold a reference to the IDWriteFontFileStream interface. The data needs to exist as long as the gfxDWriteFontFileStream exists. Not just as long as the function takes to complete.

> > > would work just as well for example.
> > At the moment, you're correct (I know this because I did what you're suggested
> > at some point :)). However I think for debugging/maintenance purposes this is a
> > bad idea. IF we do this it should be renamed Identifier() or something like
> > that. We can't go calling it 'name' and then not actually have it contain the
> > name. That would make for horrible API behaviour.
> 
> So if it's just an identifier for gfxFontCache, you can use family + face with
> no need to lowercase, no need to check for "Regular". And yes, we should call
> it Identifier and not confuse it with the actual fullname (that we're reading
> and indexing separately).

Yes, I don't mind removing those checks and all that :). But I would like to keep it showing like 'Arial Bold' etc. Just to make it easier for debugging and such.
(Assignee)

Comment 190

9 years ago
(In reply to comment #189)
> (In reply to comment #188)
> > (In reply to comment #183)
> The FontFileReference can hold a reference to the IDWriteFontFileStream
> interface. The data needs to exist as long as the gfxDWriteFontFileStream
> exists. Not just as long as the function takes to complete.
> 
Woops! Just noticed I need to properly implement AddRef and Release on that class (gfxDWriteFontFileStream) or I'll leak it.
(In reply to comment #187)
> (In reply to comment #185)
> Visual Studio, it's something obscure like Ctrl+F and then Ctrl+<something>.
> And you have to do it on a per file basis I think.

There's a global setting or option. IIRC, we document how to flip it. Let me ask around...

[10:42am] dvander: brendan: i don't have it in front of me, but it's something like
[10:42am] dvander: Tools -> Options -> Editor -> General (or maybe c/c++)
[10:43am] dvander: nothing like modelines unfortunately

/be
(In reply to comment #191)
> There's a global setting or option. IIRC, we document how to flip it. Let me
> ask around...
> 
> [10:42am] dvander: brendan: i don't have it in front of me, but it's something
> like
> [10:42am] dvander: Tools -> Options -> Editor -> General (or maybe c/c++)
> [10:43am] dvander: nothing like modelines unfortunately

[11:20am] sid0: brendan: tools -> options -> text editor -> C/C++ -> Tabs
[11:20am] brendan: sid0: thx

/be
(In reply to comment #181)
> +    static IDWriteFactory *mInstance;
> 
> Use nsRefPtr<IDWriteFactory> instead and skip the release.

Is static nsRefPtr kosher?  I didn't think static smart pointers (or static strings, or anything of that sort) played well with leak logging, or with some library-loading techniques (or something vaguely like that).
(In reply to comment #193)
> Is static nsRefPtr kosher?  I didn't think static smart pointers (or static
> strings, or anything of that sort) played well with leak logging, or with some
> library-loading techniques (or something vaguely like that).
No, it isn't, so please don't do it.
(In reply to comment #181)
> Also, there are lots of places where the 80-character per line
> rule is ignored.  We already have gfx code that ignores this in different
> places but that's a bug, we shouldn't be creating new code that does this.
 
(Drive-by...)  No such rule in gfx, I haven't used a 80-column width tty in a looooong time.  Split up lines where it makes sense to indicate flow or to separate long arguments, but don't do it unnecessarily.  100-120 column width or so is a decent guideline for when lines are becoming "too long".
(In reply to comment #195)
> (In reply to comment #181)
> > Also, there are lots of places where the 80-character per line
> > rule is ignored.  We already have gfx code that ignores this in different
> > places but that's a bug, we shouldn't be creating new code that does this.
> 
> (Drive-by...)  No such rule in gfx, I haven't used a 80-column width tty in a
> looooong time.

https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide says 80, roc wanted a standard all code (tries to) adhere to. The argument for 80 is side-by-side diffs or windows on a laptop.

Obviously there's a limit, due to the moment of inertia of one's head (big in my case) being >> one's eyes (plus limits on how far left to right your eyes can comfortably sweep, unless you are a Cylon). 120 is a bit much. SpiderMonkey went to 100 when we did the TraceMonkey work.

Sorry for more drive-by commenting, we should take this back to m.d.platform if necessary. No big deal, just wanted to plunk for 100 as the new 80, if you do not want to stick to 80.

/be
(Assignee)

Comment 197

9 years ago
(In reply to comment #192)
> (In reply to comment #191)
> > There's a global setting or option. IIRC, we document how to flip it. Let me
> > ask around...
> > 
> > [10:42am] dvander: brendan: i don't have it in front of me, but it's something
> > like
> > [10:42am] dvander: Tools -> Options -> Editor -> General (or maybe c/c++)
> > [10:43am] dvander: nothing like modelines unfortunately
> 
> [11:20am] sid0: brendan: tools -> options -> text editor -> C/C++ -> Tabs
> [11:20am] brendan: sid0: thx
> 
> /be
True, the problem is conversion. So I'm working in Cairo code at the same time, which -does- use tabs. When either I forget to switch modes, or copy-paste stuff from my cairo code. It all breaks down :-). There might be an extension to read modelines and use them, I should look that up.

Also, I'm more for 100 too, if you can't do side-by-side with 100 columns, invest in more screen real estate ;). No, but seriously, generally 80 I can live with, I changed everything to 80 now though, but in some places it looks -horrendous-.
I would defer to roc here who in the past has requested an 80-column limit.  I'm fine (in fact, happy) with longer lines but we should agree on something.  100 sounds fine with me.
I'm not sure whether 80 is the right number. I am VERY sure that 80 is our current standard and if you want to change that, the right thing to do is to discuss it on dev.platform, not here. Given that we last had that discussion not long ago IIRC, and there was no consensus to change it then, I'm also quite sure that for now hewing to 80 is the right thing to do.
> > > I don't see the need for these to be separate files, I think it would be better
> > > to keep them all in gfxDWriteFonts.h/cpp.
> > Personally I think this is a bad idea. There's a lot of places in our tree
> > where I feel the large amount of code/classes in a single file is already
> > compromising readability and making the code harder to understand.
> 
> I'm inclined to agree; I'd prefer to split rather than lump things together.
> (One of these days I'd love to split various classes out from gfxFont.h/cpp,
> for example.)

Sure, some of the classes in gfxFont.h/cpp would be better split out but "Let A Thousand Classes Bloom" is not a great philosophy either.  In this case, the files I pointed out contain what are basically helper classes so I think we should just keep them together with the code they're helping.

> > [11:20am] sid0: brendan: tools -> options -> text editor -> C/C++ -> Tabs
> > [11:20am] brendan: sid0: thx
> > 
> > /be
> True, the problem is conversion. So I'm working in Cairo code at the same time,
> which -does- use tabs. When either I forget to switch modes, or copy-paste
> stuff from my cairo code. It all breaks down :-). There might be an extension
> to read modelines and use them, I should look that up.

So run your patch through a sed one-liner to auto-convert the tabs, whatever works for you, it's not that hard to assure your patches are tab free, especially when several comments here have pointed that out (comments 54, 112, 130, 135, 147). ;)

> True, we can, GetSimulations() indicates it's a simulation for most oblique
> fonts. Despite that, however, I felt they should still be listed if there's no
> italic font available? If this is incorrect ofcourse, it's -much- easier to
> just take out all Oblique simulations.

Your code already handles simulated bold/oblique just fine, no need to insert and then immediately delete a whole bunch of fake faces.  DirectWrite is generating a *lot* of these.  I added some code using GetSimulations() to dump out the fake faces it's adding to the face list (fb = fake bold, fo = fake oblique):

Times New Roman (4 fonts)
font: Times New Roman 
font: Times New Roman Italic 
font: Times New Roman Bold 
font: Times New Roman Bold Italic 
font: Times New Roman Oblique fo 
font: Times New Roman Bold Oblique fo 

Gentium (2 fonts)
font: Gentium 
font: Gentium Italic 
font: Gentium Oblique fo 
font: Gentium Bold fb 
font: Gentium Bold Oblique fb fo 
font: Gentium Italic Bold fb 

MS Gothic (1 font)
font: MS Gothic 
font: MS Gothic Oblique fo 
font: MS Gothic Bold fb 
font: MS Gothic Bold Oblique fb fo 

> However, we can't ignore the possibility of "true" oblique faces - I
> think it's very likely that some of the sans or monospace fonts will
> include oblique faces. (Isn't that already the case for things like
> Courier New?)

I don't quite see the problem here, I doubt DirectWrite is going to tag a "real" oblique face as a "simulated" face. 

> > Rather than 'font.engine.forcegdi', use
> > 'gfx.font_rendering.directwrite.enabled' and add an entry to
> > modules/libpref/src/init/all.js in the Windows-only section.  We'll
> > need to add other conditionals like this for forcing
> > Cleartype-rendering on XP, I'd like to keep those grouped together. 
> > For initial checkin we should leave this set to 'false' and we can
> > flip it to 'true' once it's had a bit of time to get banged on.
> 
> This will be done in the final iteration where I also remove using
> Direct2D by default.

No real reason to wait on this.  The setting in all.js simply determines the default value, once you've enabled it in your profile that value will always be used.  It's important to get this right before landing; it sucks to have to change pref names later, especially if it's something that will likely go into blog postings about how to enable Direct2D support.

> > We don't need to store the localized family, this is *only* used
> > when the user opens the content prefs panel.  Better to look this up
> > within LocalizedName() rather than spend time in
> > gfxDWriteFontList::InitFontList which affects startup time.
> 
> We basically already have it. Since we've already gotten the localized
> strings set. The time spent in the lookup of the localized name is
> extremely small (we've already gotten the IDWriteLocalizedStrings set,
> so the table's already been grabbed). Do you really think it's worth
> doing this lookup in LocalizedName(), I realise this is not used a lot
> at the moment, but for future API robustness I'd say it's wiser to
> store this.

I don't understand what "future API robustness" you're talking about, you can put the exact same code into LocalizedName and then it will be called only when needed.  Why store data that's not needed *ever* for rendering content, it's only used within UI code and then only once in a blue moon?


>>>> Code like this appears several times.  Make the weight, style,
>>>> stretch part of the constructor.
>>> Shouldn't we make this constructor arguments on the baseclass then?
>>> Every implementation I've looked at seems to either add code for
>>> this in the constructor, or do what I did. Seems to me like we'd be
>>> better off adding this to the initializer list on the base class.
>> 
>> As these are all being obtained from the font, and the font is passed
>> to the constructor, it seems to me that the most logical thing is for
>> the constructor to do this internally.
> 
> Not true. In @font-face cases they're not determined by the actual font.

Have one constructor that takes the IDWriteFont and pulls out the data from that and another that takes an IDWriteFontFile along with the weight, style, stretch params.  The Mac code has something like this.
(Assignee)

Comment 201

9 years ago
(In reply to comment #200)
> > > > I don't see the need for these to be separate files, I think it would be better
> > > > to keep them all in gfxDWriteFonts.h/cpp.
> > > Personally I think this is a bad idea. There's a lot of places in our tree
> > > where I feel the large amount of code/classes in a single file is already
> > > compromising readability and making the code harder to understand.
> > 
> > I'm inclined to agree; I'd prefer to split rather than lump things together.
> > (One of these days I'd love to split various classes out from gfxFont.h/cpp,
> > for example.)
> 
> Sure, some of the classes in gfxFont.h/cpp would be better split out but "Let A
> Thousand Classes Bloom" is not a great philosophy either.  In this case, the
> files I pointed out contain what are basically helper classes so I think we
> should just keep them together with the code they're helping.
I still think the wisest thing here is moving everything to gfxDWriteCommon.h/cpp, rather than moving it into gfxWindowsDWriteFonts.cpp. File entries are cheap.

> > However, we can't ignore the possibility of "true" oblique faces - I
> > think it's very likely that some of the sans or monospace fonts will
> > include oblique faces. (Isn't that already the case for things like
> > Courier New?)
> 
> I don't quite see the problem here, I doubt DirectWrite is going to tag a
> "real" oblique face as a "simulated" face. 
> 
I believe that Jonathan means that if in that situation, we have a true oblique and an italic font, for the same weight/stretch/etc. (however unlikely). We still want to toss out that oblique font.

> > > Rather than 'font.engine.forcegdi', use
> > > 'gfx.font_rendering.directwrite.enabled' and add an entry to
> > > modules/libpref/src/init/all.js in the Windows-only section.  We'll
> > > need to add other conditionals like this for forcing
> > > Cleartype-rendering on XP, I'd like to keep those grouped together. 
> > > For initial checkin we should leave this set to 'false' and we can
> > > flip it to 'true' once it's had a bit of time to get banged on.
> > 
> > This will be done in the final iteration where I also remove using
> > Direct2D by default.
> 
> No real reason to wait on this.  The setting in all.js simply determines the
> default value, once you've enabled it in your profile that value will always be
> used.  It's important to get this right before landing; it sucks to have to
> change pref names later, especially if it's something that will likely go into
> blog postings about how to enable Direct2D support.

The problem is this will be refactored a little for it to work right. I'll disable defaulting to D2D on in my next patch, and name the prefs as we like them.

> 
> > > We don't need to store the localized family, this is *only* used
> > > when the user opens the content prefs panel.  Better to look this up
> > > within LocalizedName() rather than spend time in
> > > gfxDWriteFontList::InitFontList which affects startup time.
> > 
> > We basically already have it. Since we've already gotten the localized
> > strings set. The time spent in the lookup of the localized name is
> > extremely small (we've already gotten the IDWriteLocalizedStrings set,
> > so the table's already been grabbed). Do you really think it's worth
> > doing this lookup in LocalizedName(), I realise this is not used a lot
> > at the moment, but for future API robustness I'd say it's wiser to
> > store this.
> 
> I don't understand what "future API robustness" you're talking about, you can
> put the exact same code into LocalizedName and then it will be called only when
> needed.  Why store data that's not needed *ever* for rendering content, it's
> only used within UI code and then only once in a blue moon?
LocalizedName() would become relatively expensive. If in the future someone chooses to use it somewhere And the data is there, we have the name table loaded. I don't see any point not to do this except to save a couple of bytes per font family. To me it sounds like serious premature optimization. But, sure, if you want, easy enough to load the table again in LocalizedName().

> >>>> Code like this appears several times.  Make the weight, style,
> >>>> stretch part of the constructor.
> >>> Shouldn't we make this constructor arguments on the baseclass then?
> >>> Every implementation I've looked at seems to either add code for
> >>> this in the constructor, or do what I did. Seems to me like we'd be
> >>> better off adding this to the initializer list on the base class.
> >> 
> >> As these are all being obtained from the font, and the font is passed
> >> to the constructor, it seems to me that the most logical thing is for
> >> the constructor to do this internally.
> > 
> > Not true. In @font-face cases they're not determined by the actual font.
> 
> Have one constructor that takes the IDWriteFont and pulls out the data from
> that and another that takes an IDWriteFontFile along with the weight, style,
> stretch params.  The Mac code has something like this.
Good idea. Will do this.

Finally I wonder if we should compare DWrite's 'fake bold' and 'fake oblique' to the one cairo supplies us with. Just to see if there's a difference in appearance, and which looks better.
(Assignee)

Comment 202

9 years ago
(In reply to comment #199)
> I'm not sure whether 80 is the right number. I am VERY sure that 80 is our
> current standard and if you want to change that, the right thing to do is to
> discuss it on dev.platform, not here. Given that we last had that discussion
> not long ago IIRC, and there was no consensus to change it then, I'm also quite
> sure that for now hewing to 80 is the right thing to do.
So, the real question is, is this standard rock solid, or is it flexible when adhering to it makes code look horrible?
> > I don't quite see the problem here, I doubt DirectWrite is going to
> > tag a "real" oblique face as a "simulated" face.
> > 
> I believe that Jonathan means that if in that situation, we have a
> true oblique and an italic font, for the same weight/stretch/etc.
> (however unlikely). We still want to toss out that oblique font.

This is not a problem specific to DirectWrite, the GDI code also lumps italic and oblique faces together.  Windows never ships with italic and oblique faces in the same family.  There are some open source fonts out there that have families with both italic and oblique faces but it's extremely rare, it's not something we really need to tackle here.  Feel free to open a new bug on this.

> Finally I wonder if we should compare DWrite's 'fake bold' and 'fake
> oblique' to the one cairo supplies us with. Just to see if there's a
> difference in appearance, and which looks better.

By all means, test both ways but I would imagine DWrite's will be better since it controls the font rasterization and can do the synthetics there.  We just draw twice with a one-pixel difference so our fake bold doesn't look so bold at larger sizes.  You could increase the number of times you strike but then you get sucky performance and dealing with RGBA colors becomes a big headache (see fake bolding code in gfxFont).  You can also increase the offset but then you get weird artefacts at sharp miter joins (e.g. a capital W in Futura).
(In reply to comment #202)
> So, the real question is, is this standard rock solid, or is it flexible when
> adhering to it makes code look horrible?

The latter. As we discussed on IRC, if you have
VeryVeryVeryVeryVeryVeryVeryLongClassName::VeryVeryVeryVeryLongMethodName(VeryVeryVeryVeryLongType aParam)
there isn't much you can do (except use shorter names...)
(In reply to comment #203)
> > > I don't quite see the problem here, I doubt DirectWrite is going to
> > > tag a "real" oblique face as a "simulated" face.
> > > 
> > I believe that Jonathan means that if in that situation, we have a
> > true oblique and an italic font, for the same weight/stretch/etc.
> > (however unlikely). We still want to toss out that oblique font.
> 
> This is not a problem specific to DirectWrite, the GDI code also lumps italic
> and oblique faces together.  Windows never ships with italic and oblique faces
> in the same family.  There are some open source fonts out there that have
> families with both italic and oblique faces but it's extremely rare, it's not
> something we really need to tackle here.  Feel free to open a new bug on this.
> 
> > Finally I wonder if we should compare DWrite's 'fake bold' and 'fake
> > oblique' to the one cairo supplies us with. Just to see if there's a
> > difference in appearance, and which looks better.
> 
> By all means, test both ways but I would imagine DWrite's will be better since
> it controls the font rasterization and can do the synthetics there.  We just
> draw twice with a one-pixel difference so our fake bold doesn't look so bold at
> larger sizes.  You could increase the number of times you strike but then you
> get sucky performance and dealing with RGBA colors becomes a big headache (see
> fake bolding code in gfxFont).  You can also increase the offset but then you
> get weird artefacts at sharp miter joins (e.g. a capital W in Futura).

We should certainly not use the "draw-twice-with-offset" approach, when the font rendering engine can do better. What I'd expect is that we should tell DWrite to create its simulated styles on the fly (when we instantiate the font), like we do with GDI. But there shouldn't be any need to clutter the font list with entries for simulated styles (and end up reading the same name tables and cmaps multiple times).
(In reply to comment #203)
> > > I don't quite see the problem here, I doubt DirectWrite is going to
> > > tag a "real" oblique face as a "simulated" face.
> > > 
> > I believe that Jonathan means that if in that situation, we have a
> > true oblique and an italic font, for the same weight/stretch/etc.
> > (however unlikely). We still want to toss out that oblique font.
> 
> This is not a problem specific to DirectWrite, the GDI code also lumps italic
> and oblique faces together.  Windows never ships with italic and oblique faces
> in the same family.  There are some open source fonts out there that have
> families with both italic and oblique faces but it's extremely rare, it's not
> something we really need to tackle here.  Feel free to open a new bug on this.

So the problem is, given a family that has both oblique and italic faces, which one will be used? (It looks from a quick inspection as if the GDI code will simply use whichever of them was found first during face enumeration, and discard the other, which is not a good answer.)

Filed bug 543715 about this.
(In reply to comment #205)
> We should certainly not use the "draw-twice-with-offset" approach, when the
> font rendering engine can do better. What I'd expect is that we should tell
> DWrite to create its simulated styles on the fly (when we instantiate the
> font), like we do with GDI. But there shouldn't be any need to clutter the font
> list with entries for simulated styles (and end up reading the same name tables
> and cmaps multiple times).

Totally agree.
(Assignee)

Comment 208

9 years ago
(In reply to comment #207)
> (In reply to comment #205)
> > We should certainly not use the "draw-twice-with-offset" approach, when the
> > font rendering engine can do better. What I'd expect is that we should tell
> > DWrite to create its simulated styles on the fly (when we instantiate the
> > font), like we do with GDI. But there shouldn't be any need to clutter the font
> > list with entries for simulated styles (and end up reading the same name tables
> > and cmaps multiple times).
> 
> Totally agree.
As per discussion with jfkthame on IRC. Since DWrite does not let you add simulations to an IDWriteFont, just an IDWriteFontFile, we're leaving the bold simulated entries in the font list for now.
(Assignee)

Updated

9 years ago
Depends on: 543892
Testcase for synthetic rendering using various Windows 7 fonts that lack bold and/or italic faces:

http://people.mozilla.org/~jdaggett/tests/simplesynthetics.html

Compare with trunk build to see correct rendering.
(Assignee)

Comment 210

9 years ago
Created attachment 424920 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v14

Latest Thebes Patch. This seems to render everything as we want it. I created an additional bug and patch for making font_matrix transforms work correctly for both D2D and GDI. To summarize the following patches need to be applied to your tree for everything to work right:

Thebes patch from this bug
Widget patch from this bug
bug 534787
bug 543892
And USE_WIN_SURFACE must be undefined in modules/libpr0n

In the latest patch, D2D and DirectWrite are no longer automatically on, in order to switch set the following prefs manually:
gfx.font_rendering.directwrite.enabled to true (if this is not enabled Direct2D will -not- be used)
mozilla.widget.render-mode to 6 (RENDER_DIRECT2D)
Attachment #424484 - Attachment is obsolete: true
Attachment #424484 - Flags: review?(jfkthame)
Attachment #424484 - Flags: review?(jdaggett)
(Assignee)

Updated

9 years ago
Attachment #424920 - Flags: review?(jfkthame)
Attachment #424920 - Flags: review?(jdaggett)
(In reply to comment #210)
> And USE_WIN_SURFACE must be undefined in modules/libpr0n

This needs to be part of a patch somewhere, either on this bug or another bug if that's more appropriate, so that it can be reviewed and landed.
Did a quick pass on the Thebes v14 patch and noticed a few things:

+        mContext->Rectangle(gfxRect(gfxPoint(0.0, 0.0), 
+                            gfxSize(NS_ceil(mNativeRect.size.width), 
+                                    NS_ceil(mNativeRect.size.height))));
 
         nsRefPtr<gfxPattern> pat = new gfxPattern(alphaSurface);
 
+	pat->SetFilter(gfxPattern::FILTER_NEAREST);

Tab.

+class DWriteFactory
+{
+public:
+    static IDWriteFactory *Instance()

I really think this belongs in an nsRefPtr<IDWriteFactory> in gfxWindowsPlatform, that's where other singleton objects like this are put.  Right now you grab the instance but you never release it, so it will leak at shutdown.

+    if (tableContext) {
+        fontFace->ReleaseFontTable(&tableContext);
+    }

Lordy, what a horrible API... (just kvetching...)

+gfxFont *
+gfxWindowsDWriteFontEntry::CreateFontInstance(const gfxFontStyle* aFontStyle,
+                                              PRBool /*aNeedsBold*/)
+{
+    gfxFont *newFont;
+    newFont = new gfxWindowsDWriteFont(this, aFontStyle);
+    if (!newFont) {
+        return nsnull;
+    }
+    if (!newFont->Valid()) {
+        delete newFont;
+        return nsnull;
+    }
+    nsRefPtr<gfxFont> font = newFont;
+    gfxFontCache::GetCache()->AddNew(font);
+    return newFont;
+}

Argh, so there's some bad refactoring going on here, probably part of the recent Windows refactoring that I missed when reviewing.

The code in the gfxFontGroup looks up fonts in the group.  If it doesn't find any of the fonts in the list, it adds the default font.  It calls gfxFontEntry::GetOrMakeFont (not virtual!) which calls the virtualized CreateFontInstance method.  Problem is that both gfxFontEntry::GetOrMakeFont and the multiple derived versions of this method (DWrite, GDI, Mac) are adding the font to the font cache, so you'll get a double add.  Not sure whether that will cause a leak or not but it certainly is not good.  

Similar code seems to exist in static methods in gfxWindowsFont::GetOrMakeFont and gfxWindowsDWriteFont::GetOrMakeFont.  This code needs to use gfxFontEntry::GetOrMakeFont and the gfxFontEntry really, really, really needs another name to make it easier to distinguish from the static methods in gfxFont subclasses. 

We should probably do a quick fix here or in a separate bug that blocks this one.

+    hr = DWriteFactory::Instance()->CreateCustomFontFileReference(
+        fontData,
+        fontLength,
+        gfxDWriteFontFileLoader::Instance(),
+        getter_AddRefs(fontFile));

+gfxDWriteFontFileStream::gfxDWriteFontFileStream(PRUint8 *aData,
+                                                 PRUint32 aLength)
+{
+    mData = new PRUint8[aLength];
+    mDataSize = aLength;
+    memcpy(mData, aData, aLength);
+}

I think this is a poor way of using this API.  The API is asking you to pass an application-defined "key" along with the size of that key.  Seems like it will be copied around internally within DWrite code and that's sucky.  I think you should at least be able to pass a pointer to a nsTArray and adopt the memory using SwapElements within gfxDWriteFontFileStream.  That way you don't copy around large data blobs, just pointers to those blobs.
Comment on attachment 424920 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v14

> -EXPORTS +=	gfxWindowsFonts.h 
> +EXPORTS +=  gfxWindowsFonts.h

A small nit - in general, it's preferable to avoid touching existing lines if there's no real reason to change them, for the sake of hg blame.

(In this case, I suspect de-tabbing is the culprit. Note that tabs *are* used in Makefiles, unlike the codebase in general.)

> -CPPSRCS	+= gfxWindowsFonts.cpp \
> +CPPSRCS	+= gfxWindowsDWriteFonts.cpp \
> +	   gfxWindowsFonts.cpp \

Here also, the existing gfxWindowsFonts.cpp line didn't need to be touched.

> +void
> +gfxWindowsDWriteFontFamily::FindStyleVariations()
> +{
> +    HRESULT hr;
> +    if (mHasStyles) {
> +        return;
> +    }
> +    mHasStyles = PR_TRUE;
> +    mAvailableFonts.Clear();

Isn't the Clear() redundant here?


> +gfxFontEntry *
> +gfxDWriteFontList::GetDefaultFont(const gfxFontStyle *aStyle,
> +                                  PRBool &aNeedsBold)
> +{
> +    // ...but just in case, try another approach as well

Oops, this comment (copied from the GDI code) isn't appropriate here!


> +    /**
> +     * A fontentry only needs to have either of these. If it has both only
> +     * the IDWriteFont will be used.
> +     */
> +    nsRefPtr<IDWriteFont> mFont;
> +    nsRefPtr<IDWriteFontFile> mFontFile;

Could we have an assertion somewhere to check this, as a safeguard during future revision?

> +void TextAnalysis::SetCurrentRun(UINT32 textPosition)
> +{
> +    // Move the current run to the given position.
> +    // Since the analyzers generally return results in a forward manner,
> +    // this will usually just return early. If not, find the
> +    // corresponding run for the text position.
> +
> +    if (mCurrentRun && mCurrentRun->ContainsTextPosition(textPosition)) {
> +        return;
> +    }
> +
> +    for (Run *run = mRunHead; run; run = run->nextRun) {
> +        if (run->ContainsTextPosition(textPosition)) {
> +            mCurrentRun = run;
> +            return;
> +        }
> +    }
> +}

I assume this should always find the given position? If so, I'd suggest adding an NS_NOTREACHED assertion at the end of the function, so that we'll be alerted if that assumption gets broken.


> +// Chosen this as to resemble DWrite's own oblique face style.
> +#define OBLIQUE_SKEW_FACTOR 0.3

Ewwww.... does it skew that much? I'd be interested to see screenshots, but in general I think 0.3 is excessive. We currently use 0.25 on the Mac; some hand-drawn oblique faces I've looked at are slightly less slanted than this, even.

> +    for(unsigned int i = 0; i < ranges.Length(); i++) {

nit: space^ needed; same again a few lines later.

> +        if (ranges[i].Length() > MAX_RANGE_LENGTH) {
> +            ranges.InsertElementAt(i + 1, 
> +                                   gfxTextRange(ranges[i].start 
> +                                     + MAX_RANGE_LENGTH,
> +                                   ranges[i].end));
> +            ranges[i + 1].font = ranges[i].font;
> +            ranges[i].end = ranges[i].start + MAX_RANGE_LENGTH;
> +        }

Suggest adding a TODO note about finding logical places to split ranges if this is necessary; we don't want to be splitting within a character cluster where OpenType shaping is needed, as that will presumably prevent proper analysis.


> +        hr = analyzer->GetGlyphs(aString + range.start, range.Length(),
> +            font->mFontFace, FALSE, readingDirection == 
> +            DWRITE_READING_DIRECTION_RIGHT_TO_LEFT,
> +            &runHead->mScript, NULL, NULL, NULL, NULL, 0,
> +            maxGlyphs, clusters.Elements(), textProperties.Elements(),
> +            indices.Elements(), glyphProperties.Elements(), &actualGlyphs);
> +        if (FAILED(hr)) {
> +            NS_WARNING("Analyzer failed to get glyphs.");
> +            continue;
> +        }

This really needs to handle the case where the initial maxGlyphs setting isn't large enough. (MSDN says "In the event that maxGlyphCount is not big enough, HRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER) will be returned. The application should allocate a larger buffer and try again.")

> +        hr = analyzer->GetGlyphPlacements(aString + range.start, 
> +                                          clusters.Elements(),
> +                                          textProperties.Elements(),
> +                                          range.Length(),
> +                                          indices.Elements(),
> +                                          glyphProperties.Elements(),
> +                                          actualGlyphs,
> +                                          font->mFontFace,
> +                                          (float)mStyle.size,
> +                                          FALSE,
> +                                          FALSE,
> +                                          &runHead->mScript,
> +                                          NULL,
> +                                          NULL,
> +                                          NULL,
> +                                          0,
> +                                          advances.Elements(),
> +                                          glyphOffsets.Elements());
> +        if (FAILED(hr)) {
> +            NS_WARNING("Analyzer failed to get glyph placements.");
> +            continue;
> +        }

Ditto.

> +	pat->SetFilter(gfxPattern::FILTER_NEAREST);

Oops, tab!
(In reply to comment #213)
> (From update of attachment 424920 [details] [diff] [review])
> > -EXPORTS +=	gfxWindowsFonts.h 
> > +EXPORTS +=  gfxWindowsFonts.h
> 
> A small nit - in general, it's preferable to avoid touching existing lines if
> there's no real reason to change them, for the sake of hg blame.
> 
> (In this case, I suspect de-tabbing is the culprit. Note that tabs *are* used
> in Makefiles, unlike the codebase in general.)

Note that we should only be using tabs in the commands list of rules in Makefiles, where they're required, and not anywhere else. Some detabification along the way isn't too bad, as long as it's nearby to other changes.
(Assignee)

Comment 215

9 years ago
(In reply to comment #213)
> (From update of attachment 424920 [details] [diff] [review])
> > +        hr = analyzer->GetGlyphPlacements(aString + range.start, 
> > +                                          clusters.Elements(),
> > +                                          textProperties.Elements(),
> > +                                          range.Length(),
> > +                                          indices.Elements(),
> > +                                          glyphProperties.Elements(),
> > +                                          actualGlyphs,
> > +                                          font->mFontFace,
> > +                                          (float)mStyle.size,
> > +                                          FALSE,
> > +                                          FALSE,
> > +                                          &runHead->mScript,
> > +                                          NULL,
> > +                                          NULL,
> > +                                          NULL,
> > +                                          0,
> > +                                          advances.Elements(),
> > +                                          glyphOffsets.Elements());
> > +        if (FAILED(hr)) {
> > +            NS_WARNING("Analyzer failed to get glyph placements.");
> > +            continue;
> > +        }
> 
> Ditto.
That API can't return such a failure if you're referring to max_glyphs being too small.
(Assignee)

Comment 216

9 years ago
Created attachment 425110 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v15

Quick processing of current review comments on v14. Haven't moved the singleton around yet as I'm not sure yet what to do there. I have re-added the release in gfxWindowsPlatform destruction. As the nsRefPtr isn't an option from what I understand.
Attachment #424920 - Attachment is obsolete: true
Attachment #425110 - Flags: review?(jfkthame)
Attachment #425110 - Flags: review?(jdaggett)
Attachment #424920 - Flags: review?(jfkthame)
Attachment #424920 - Flags: review?(jdaggett)
(In reply to comment #216)
> Created an attachment (id=425110) [details]
> Thebes DirectWrite and Direct2D Code v15
> 
> Quick processing of current review comments on v14. Haven't moved the singleton
> around yet as I'm not sure yet what to do there. I have re-added the release in
> gfxWindowsPlatform destruction. As the nsRefPtr isn't an option from what I
> understand.

Subclasses of gfxPlatform are always singletons, so a nsRefPtr within gfxWindowsPlatform (non-static) is the effectively the same as a static.  Shouldn't be any static load order problems.
Comment on attachment 425110 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v15

> +    /**
> +     * We pass in a -reference- to this pointer to newFontData. DWrite will
> +     * internally copy what is at that pointer, and pass that to
> +     * CreateStreamFromKey.
> +     */
> +    nsTArray<PRUint8> *data = &newFontData;
> +    hr = DWriteFactory::Instance()->CreateCustomFontFileReference(
> +        &data,
> +        sizeof(&data),
> +        gfxDWriteFontFileLoader::Instance(),
> +        getter_AddRefs(fontFile));

I think it'd be good to add a comment that newFontData will no longer contain the actual font data after this call, because it will have been transferred to gfxDWriteFontFileStream's internal array.


> +    DWRITE_FONT_SIMULATIONS sims = DWRITE_FONT_SIMULATIONS_NONE;
> +    if ((GetStyle()->style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) &&
> +        !fe->IsItalic()) {
> +        if (fe->mFontFile) {
> +            sims |= DWRITE_FONT_SIMULATIONS_OBLIQUE;
> +        } else {
> +            // If we're from an IDWriteFont we need to use the font_matrix
> +            // to apply the oblique simulation.
> +            mNeedsOblique = PR_TRUE;
> +        }
> +    }

Would it make sense to simply use the font_matrix approach in all cases, for uniformity?


> +        UINT32 maxGlyphs = 0;
> +trymoreglyphs:
> +        if ((PR_UINT32_MAX - 3 * range.Length() / 2 + 16) < maxGlyphs) {
> +            // This isn't going to work, we're going to cross the UINT32 upper
> +            // limit. Next range it is.
> +            continue;
> +        }

Hard to imagine we'd ever really hit that limit, but I guess some kind of malicious font might try!

I wonder, though, if it would be better to shorten the range rather than increase maxGlyphs, thinking of your note earlier about 16-bit limits inside the analyzer. With a range length of 25K, a font that replaces lots of single characters with 3-glyph sequences could hit the 16-bit barrier.


> +        nsAutoTArray<UINT16, 400> clusters;
> +        nsAutoTArray<UINT16, 400> indices;
> +        if (!clusters.SetLength(range.Length())) {
> +            continue;
> +        }
> +        if (!indices.SetLength(maxGlyphs)) {
> +            continue;
> +        }
> +        nsAutoTArray<DWRITE_SHAPING_TEXT_PROPERTIES, 400> textProperties;
> +        nsAutoTArray<DWRITE_SHAPING_GLYPH_PROPERTIES, 400> glyphProperties;
> +        if (!textProperties.SetLength(maxGlyphs)) {
> +            continue;
> +        }
> +        if (!glyphProperties.SetLength(maxGlyphs)) {
> +            continue;
> +        }

Just a suggestion, how about rearranging this more compactly as:

+        nsAutoTArray<UINT16, 400> clusters;
+        nsAutoTArray<UINT16, 400> indices;
+        nsAutoTArray<DWRITE_SHAPING_TEXT_PROPERTIES, 400> textProperties;
+        nsAutoTArray<DWRITE_SHAPING_GLYPH_PROPERTIES, 400> glyphProperties;
+        if (!clusters.SetLength(range.Length()) ||
+            !indices.SetLength(maxGlyphs) ||
+            !textProperties.SetLength(maxGlyphs) ||
+            !glyphProperties.SetLength(maxGlyphs)) {
+            continue;
+        }


> +        nsAutoTArray<FLOAT, 400> advances;
> +        nsAutoTArray<DWRITE_GLYPH_OFFSET, 400> glyphOffsets;
> +        if (!advances.SetLength(actualGlyphs)) {
> +            continue;
> +        }
> +        if (!glyphOffsets.SetLength(actualGlyphs)) {
> +            continue;
> +        }

Similarly, could save a couple of lines here by combining the 'if's.

 
>  gfxWindowsPlatform::~gfxWindowsPlatform()
>  {
>      // not calling FT_Done_FreeType because cairo may still hold references to
>      // these FT_Faces.  See bug 458169.
> +    // Release this here to avoid leakage - not the best thing to do.
> +    // TODO: Look where we can move this and how to do this.
> +    if (DWriteFactory::Instance()) {
> +        DWriteFactory::Instance()->Release();
> +    }

Should this be within an #ifndef MOZ_FT2_FONTS? (And/or CAIRO_HAS_DWRITE_FONT?)
(Assignee)

Comment 219

9 years ago
(In reply to comment #218)
> (From update of attachment 425110 [details] [diff] [review])
> > +        UINT32 maxGlyphs = 0;
> > +trymoreglyphs:
> > +        if ((PR_UINT32_MAX - 3 * range.Length() / 2 + 16) < maxGlyphs) {
> > +            // This isn't going to work, we're going to cross the UINT32 upper
> > +            // limit. Next range it is.
> > +            continue;
> > +        }
> 
> Hard to imagine we'd ever really hit that limit, but I guess some kind of
> malicious font might try!
> 
> I wonder, though, if it would be better to shorten the range rather than
> increase maxGlyphs, thinking of your note earlier about 16-bit limits inside
> the analyzer. With a range length of 25K, a font that replaces lots of single
> characters with 3-glyph sequences could hit the 16-bit barrier.
I don't think there's such a 16-bit limit on the amount of glyphs output.
(Assignee)

Comment 220

9 years ago
(In reply to comment #211)
> (In reply to comment #210)
> > And USE_WIN_SURFACE must be undefined in modules/libpr0n
> 
> This needs to be part of a patch somewhere, either on this bug or another bug
> if that's more appropriate, so that it can be reviewed and landed.

Just in case anyone else missed this, bug 534788 covers this issue.

Updated

9 years ago
Depends on: 545317

Updated

9 years ago
No longer depends on: 535396
Bug 532106 doesn't need to block this (per Bas).
No longer depends on: 532106
Created attachment 426166 [details] [diff] [review]
patch, add pref to all.js

Adds pref to all.js for gfx.font_rendering.directwrite.enabled (disabled by default).
(Assignee)

Comment 223

9 years ago
(In reply to comment #217)
> (In reply to comment #216)
> > Created an attachment (id=425110) [details] [details]
> > Thebes DirectWrite and Direct2D Code v15
> > 
> > Quick processing of current review comments on v14. Haven't moved the singleton
> > around yet as I'm not sure yet what to do there. I have re-added the release in
> > gfxWindowsPlatform destruction. As the nsRefPtr isn't an option from what I
> > understand.
> 
> Subclasses of gfxPlatform are always singletons, so a nsRefPtr within
> gfxWindowsPlatform (non-static) is the effectively the same as a static. 
> Shouldn't be any static load order problems.
Hrm, been looking at this, this means every time we access the DWriteFactory we need to execute a GetPlatform, since this function isn't in the header directly it probably won't inline that well. This means if we do this every access of the DWrite factory will effectively cause a call. This may be a problem?
Surely not. It just returns the global singleton. In fact surely GetPlatform should just be inline...
(Assignee)

Comment 225

9 years ago
Created attachment 427551 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v16

Updated the Thebes patch.
Attachment #425110 - Attachment is obsolete: true
Attachment #427551 - Flags: review?(jfkthame)
Attachment #427551 - Flags: review?(jdaggett)
Attachment #425110 - Flags: review?(jfkthame)
Attachment #425110 - Flags: review?(jdaggett)
(Assignee)

Comment 226

9 years ago
(In reply to comment #224)
> Surely not. It just returns the global singleton. In fact surely GetPlatform
> should just be inline...

Perhaps we should define it in the header then, gfxPlatform::GetPlatform()? That should inline it better.
(Assignee)

Comment 227

9 years ago
Created attachment 427836 [details] [diff] [review]
Libpr0n patch to use image surfaces with D2D

Rather than changing the USE_WIN_SURFACE define. We can just return true for ShouldUseImageSurface when using Direct2D rendering. This patch does that. This invalidates bug 534788.
Attachment #427836 - Flags: review?(jmuizelaar)
Comment on attachment 427551 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v16

Would you mind doing a global replace of "gfxWindowsDWrite" with "gfxDWrite" throughout the patch? Seems like that would be just as clear, and it'd make code formatting (now and for the future) that little bit easier.

A few minor things I noticed reading through this:

+    /**
+     * Initialize the font list, this populates mFontEntries.

No it doesn't; mFontEntries appears to be unused, as far as I can see.

+    /** List of font entries for the fonts in this font group */
+    nsTArray<nsRefPtr<gfxFontEntry> > mFontEntries;

Redundant; fonts (not font entries) are stored into the superclass's mFonts array.


+    *fontFileStream = 
+        new gfxDWriteFontFileStream(*(nsTArray<PRUint8>**)fontFileReferenceKey);
+
+    if (!fontFileStream) {
+        return E_OUTOFMEMORY;
+    }

I think you meant "if (!*fontFileStream)..." here. However, unless you use "new (std::nothrow)" there's no need for the check, as plain "new" cannot return NULL, it will throw an exception (and kill the app) if it allocation fails.


+    aLocalizedName = NS_LITERAL_STRING("Unknown Font");

I think aLocalizedName.AssignLiteral("...") is the preferred way.

+        localeName = NS_LITERAL_STRING("en-us");

Ditto.


+    if (!fe) {
+        return nsnull;
+    }
+
+    return fe;

The "if...." looks redundant here.


+    fontFile->Analyze(&isSupported, &fileType, &entry->mFaceType, &numFaces);
+    if (!isSupported) {
+        delete entry;
+        return nsnull;
+    }

I think we should also reject files where numFaces != 1, as we have no way of specifying which face to use in such a case. (AFAIK, we don't support .ttc files as user fonts on any platform at the moment.)


+        hr = analyzer->GetGlyphs(aString + range.start, range.Length(),
+            font->mFontFace, FALSE, readingDirection == 
+            DWRITE_READING_DIRECTION_RIGHT_TO_LEFT,
+            &runHead->mScript, NULL, NULL, NULL, NULL, 0,
+            maxGlyphs, clusters.Elements(), textProperties.Elements(),
+            indices.Elements(), glyphProperties.Elements(), &actualGlyphs);

Reformat to keep "readingDirection == DWRITE_READING_DIRECTION_RIGHT_TO_LEFT" together on one line, please.


+        nsAutoTArray<gfxTextRun::DetailedGlyph,1> detailedGlyphs;
+        for (unsigned int c = 0; c < range.Length(); c++) {
+            if (c > 0 && clusters[c] == clusters[c - 1]) {
+                g.SetComplex(glyphProperties[i].isClusterStart, PR_FALSE, 0);
+                textRun->SetGlyphs(range.start + c, g, nsnull);
+            } else {

It'd be nice to have a comment here to say what's happening (this character is a cluster continuation, so it has no glyphs of its own, or something like that); then use "continue", and the "else" can be dropped and one level of indent removed for the rest of the loop.


+// DirectWrite is not available on all search for function pointer.

I know what you mean but the comment seems incomplete... "is not available on all versions of Windows, so..." or something.

Comment 229

9 years ago
I got to sneak in a small question in between here, is the plan to have this activated by default in Firefox.next, if it gets finished in time?
@Magne - I think that's the plan, but we'll know more once it's checked in and gets wider testing.
(Assignee)

Comment 231

9 years ago
(In reply to comment #228)
> (From update of attachment 427551 [details] [diff] [review])
> Would you mind doing a global replace of "gfxWindowsDWrite" with "gfxDWrite"
> throughout the patch? Seems like that would be just as clear, and it'd make
> code formatting (now and for the future) that little bit easier.

I'm quite font of that idea! We should probably raise an issue then to replace gfxWindowsFonts with gfxGDIFonts to avoid future confusion though.

> 
> A few minor things I noticed reading through this:
> 
> +    /**
> +     * Initialize the font list, this populates mFontEntries.
> 
> No it doesn't; mFontEntries appears to be unused, as far as I can see.
> 
> +    /** List of font entries for the fonts in this font group */
> +    nsTArray<nsRefPtr<gfxFontEntry> > mFontEntries;
> 
> Redundant; fonts (not font entries) are stored into the superclass's mFonts
> array.

Indeed, remnants of the old font code. I'll remove them.

> 
> 
> +    *fontFileStream = 
> +        new
> gfxDWriteFontFileStream(*(nsTArray<PRUint8>**)fontFileReferenceKey);
> +
> +    if (!fontFileStream) {
> +        return E_OUTOFMEMORY;
> +    }
> 
> I think you meant "if (!*fontFileStream)..." here. However, unless you use "new
> (std::nothrow)" there's no need for the check, as plain "new" cannot return
> NULL, it will throw an exception (and kill the app) if it allocation fails.

You are right on both accounts. I thought coding guidelines were still to if check though! I'll remove it.
(In reply to comment #231)

> > Would you mind doing a global replace of "gfxWindowsDWrite" with "gfxDWrite"
> > throughout the patch? Seems like that would be just as clear, and it'd make
> > code formatting (now and for the future) that little bit easier.
> 
> I'm quite font of that idea! We should probably raise an issue then to replace
> gfxWindowsFonts with gfxGDIFonts to avoid future confusion though.

Something like that is going to happen anyway in the restructuring in bug 502906.
We still use non-throwing new, so you need to check for null.
(In reply to comment #233)
> We still use non-throwing new, so you need to check for null.

I don't think this is correct - at least on Windows, new does throw if allocation fails (see, for example, bug 545989 comment #2). Note that this is different from malloc behavior, where I don't think we have switched to an infallible version yet.
(In reply to comment #234)
> (In reply to comment #233)
> > We still use non-throwing new, so you need to check for null.
> I don't think this is correct - at least on Windows, new does throw if
> allocation fails (see, for example, bug 545989 comment #2). Note that this is
> different from malloc behavior, where I don't think we have switched to an
> infallible version yet.
We are still supposed to check the result of new until we figure out what to do in bug 427099 and land it.
Comment on attachment 427551 [details] [diff] [review]
Thebes DirectWrite and Direct2D Code v16

r=jfkthame, assuming nits from comment 228 are handled. Let's get this into the tree! :)
Attachment #427551 - Flags: review?(jfkthame) → review+
Comment on attachment 427836 [details] [diff] [review]
Libpr0n patch to use image surfaces with D2D

kGDIObjectsHighWaterMark should be defined below the check for GetRenderMode()
Attachment #427836 - Flags: review?(jmuizelaar) → review+
Looks good, I think this is close, needs just a couple more changes.  As noted by others, need to check for new xxx coming back null.  And as we discussed on IRC, several methods of gfxWindowsDWriteFontGroup duplicate functionality that Jonathan moved into gfxFontGroup as part of work for bug 502906, those just need to be trimmed out.  Don't worry about the GDI code, that will be reworked elsewhere.

One nit:

+/**
+ * \brief Class representing a font face for a font entry.
+ */

+    /**
+     * Gets the font at a certain position in the font list, it will
+     * create this font if it hasn't actually been created yet.
+     *
+     * \param i Position in the font list
+     * \return Object representing the font
+     */

The documentation here is inconsistent, you've included it in some places but omitted it in others.  I would suggest only including these comments where the complexity warrants and omitting the verbose parameter descriptions which are almost identical to parameter names.  And the pattern in other code seems to be @param, @return and not \param, \return.
(Assignee)

Comment 239

9 years ago
Direct2D code landed in changeset 0e1c0b71bf13 and 4bc737467770. Some follow-up fixes and changes are still required though.
Let's get those followups in their own, narrow bugs, then: the investigation is clearly FIXED, and it'll be very hard to track status and progress if we just do everything in one bug of ill-defined scope.
And, BTW, this is awesome!

We should tell people how to activate it, and get the word out, so that we can start getting feedback on it.

Comment 242

9 years ago
I've been following this bug for a while and I'm glad to see that the hard work is finally done.

(In reply to comment #241)
> We should tell people how to activate it, and get the word out, so that we can
> start getting feedback on it.

Just to clarify, could you tell us here how is it done?
(Assignee)

Comment 243

9 years ago
Actually Shaver is right. This is fixed, and I'm marking it as such. Removed the two blocking issues as they are follow-up issues we need to fix, and not blocking this 'investigation' (erm, maybe that was a poor choice all along really :-)).

I will soon work up a blog post explaining the world how to turn this on in more layman's terms, and what issues to expect and such. For now, the bottom line is getting two prefs into your prefs.js file:

gfx.font_rendering.directwrite.enabled must be set to true.
mozilla.widget.render-mode must be set to 6(RENDER_DIRECT2D)
Status: NEW → RESOLVED
Last Resolved: 9 years ago
No longer depends on: 548935, 548942
Resolution: --- → FIXED
I'm not getting an autoscroll popup with those settings, is that expected?
Followup bugs (i.e. bugs caused by the landing) *should* be marked as blocking this bug. It doesn't necessarily make much sense conceptually, but it's a convention we use consistently across bugzilla.
Regressions from this bug should be marked as blocking, but unfinished features of the work should be bugs that depend on it, or not related at all. IMO!
Depends on: 548962
I filed bug 548962 for the autoscroll popup not showing up issue.

Updated

9 years ago
Depends on: 548964

Updated

9 years ago
Depends on: 548965
Depends on: 548967

Updated

9 years ago
Depends on: 548969

Updated

9 years ago
Depends on: 548975

Updated

9 years ago
Depends on: 548977

Updated

9 years ago
Depends on: 548985
Depends on: 548983

Updated

9 years ago
Depends on: 548987
Which SDK do you need to be able to compile this? Are we dropping support for older SDKs, or was --with-windows-version support overlooked?
Yes, this completely broke building with older SDKs
(In reply to comment #249)
> Yes, this completely broke building with older SDKs

Could you log a bug on this?
(In reply to comment #243)
> I will soon work up a blog post explaining the world how to turn this on in
> more layman's terms, and what issues to expect and such. For now, the bottom
> line is getting two prefs into your prefs.js file:
> 
> gfx.font_rendering.directwrite.enabled must be set to true.
> mozilla.widget.render-mode must be set to 6(RENDER_DIRECT2D)

The 'mozilla.widget.render-mode' pref should be added to all.js.  Maybe a better name (i.e. the 'mozilla' prefix feel archaic)?  Whatever I guess, the important part is that it's in all.js.  We should never be asking anyone to edit prefs.js.

Steps to enable Direct2D/DirectWrite support:

1. Enter 'about:config'
2. Click through the warning, if necessary
3. Enter gfx.font in the 'Filter' box
4. Double-click on 'gfx.font_rendering.directwrite.enabled' to set it to true
5. Below this, right click and select New > Integer to add a pref setting
6. Enter 'mozilla.widget.render-mode' for the preference name, 6 for the value
7. Restart

To disable, set gfx.font_rendering.directwrite.enabled to false, delete mozilla.widget.render-mode, then restart.
Depends on: 549019
Depends on: 549056
Blocks: 375889
Blocks: 311534
Blocks: 448263
Blocks: 403447
Blocks: 421587
Blocks: 549065
(In reply to comment #251)


> Steps to enable Direct2D/DirectWrite support:
> 
> 1. Enter 'about:config'
> 2. Click through the warning, if necessary
> 3. Enter gfx.font in the 'Filter' box
> 4. Double-click on 'gfx.font_rendering.directwrite.enabled' to set it to true
> 5. Below this, right click and select New > Integer to add a pref setting
> 6. Enter 'mozilla.widget.render-mode' for the preference name, 6 for the value
> 7. Restart
> 

Do you need a special video card to see all the new bugs? I added the prefs
gfx.font_rendering.directwrite.enabled boolean true
and
mozilla.widget.render-mode integer 6
but everything still the same.

Comment 253

9 years ago
(In reply to comment #252)
> (In reply to comment #251)
> 
> 
> > Steps to enable Direct2D/DirectWrite support:
> > 
> > 1. Enter 'about:config'
> > 2. Click through the warning, if necessary
> > 3. Enter gfx.font in the 'Filter' box
> > 4. Double-click on 'gfx.font_rendering.directwrite.enabled' to set it to true
> > 5. Below this, right click and select New > Integer to add a pref setting
> > 6. Enter 'mozilla.widget.render-mode' for the preference name, 6 for the value
> > 7. Restart
> > 
> 
> Do you need a special video card to see all the new bugs? I added the prefs
> gfx.font_rendering.directwrite.enabled boolean true
> and
> mozilla.widget.render-mode integer 6
> but everything still the same.

You need Windows Vista or Windows 7, at least.
Yeah, it's Vista with NVIDIA GeForce 9200M GS.
Guess I'm doomed to miss all the bugs. No black holes, slow loading pages, wrong fonts etc. Sigh.
Hm, your hardware should meet the requirements. Do you have SP2 and the subsequent platform update installed, and are your graphics drivers up-to-date?

Comment 256

9 years ago
What add-ons are you using? I've noticed that the Strata40 add-ons family stops Direct2D from being used.
(Assignee)

Comment 257

9 years ago
(In reply to comment #254)
> Yeah, it's Vista with NVIDIA GeForce 9200M GS.
> Guess I'm doomed to miss all the bugs. No black holes, slow loading pages,
> wrong fonts etc. Sigh.

Right now the hardware requirement is actually set quite low. We might raise it to DX10 and up cards only. But I wanted to get the broadest possible view of what kind of issues people are having.

Comment 258

9 years ago
I disagree. It works fine for me with my built-in DX9 chip-set.

Maybe you should enable it by default for DX10 and above, but disable it for DX9 cards (and allow them to enable it in the Options). Or is that what you meant?

Updated

9 years ago
No longer blocks: 549065
Depends on: 549065
NVIDEA NVIDIA GeForce 9200M GS  
date 23-7-2009
driver version 8.15.11.8644
The same with NVIDEA GeForce Go 7300. Don't see any issue.

(In reply to comment #257)
> (In reply to comment #254)
> > Yeah, it's Vista with NVIDIA GeForce 9200M GS.
> > Guess I'm doomed to miss all the bugs. No black holes, slow loading pages,
> > wrong fonts etc. Sigh.
> 
> Right now the hardware requirement is actually set quite low. We might raise it
> to DX10 and up cards only. But I wanted to get the broadest possible view of
> what kind of issues people are having.

Sorry, you mean it is expected with these cards? Then I don't need to try the third computer :).

Comment 261

9 years ago
I can't enable it here either, I set the DirectWrite preference to true as well as setting the render-mode preference to 6, restarted a couple of times as well.

The preview builds worked fine for me, I'm on Windows 7 x64 with an Nvidia 8600GT (Nvidia seems to be a common factor)
(Assignee)

Comment 262

9 years ago
Hmm, I'm just looking at the code and in theory if your hardware -really- doesn't support the low requirements that I'm already having your build might even crash. But a 8600GT should definitely work I think. So that's not what you're seeing. I'm unsure why it would not enable it for you... I'll let you know if I have any ideas, are you sure it's not working?

Comment 263

9 years ago
Now I got around to start my PC too. Another one with Nvidia, another one where nothing changes. It did work with the original tryserver build though, where I could see changes in the text/fonts and several bugs that have been mentioned here. But with the current trunk and these settings, it just looks normal.
(Assignee)

Comment 264

9 years ago
Are you sure you have:

A. A recent trunk build (or last night's nightly seems to work)
B. In your prefs.js
user_pref("gfx.font_rendering.directwrite.enabled", true);
user_pref("mozilla.widget.render-mode", 6);

Make sure that you set these prefs once you've completely shut it down, on the right profile. The shutdown will overwrite the prefs in your profile! So you have to save the prefs.js file while no firefox instance is running for that profile.

Comment 265

9 years ago
(In reply to comment #264)
> Are you sure you have:
> 
> A. A recent trunk build (or last night's nightly seems to work)
> B. In your prefs.js
> user_pref("gfx.font_rendering.directwrite.enabled", true);
> user_pref("mozilla.widget.render-mode", 6);
> 
> Make sure that you set these prefs once you've completely shut it down, on the
> right profile. The shutdown will overwrite the prefs in your profile! So you
> have to save the prefs.js file while no firefox instance is running for that
> profile.

A. Latest possible, downloaded from http://nightly.mozilla.org.
B. The preferences are right there, in my only profile on this computer, intact, just as they look in your post.

Comment 266

9 years ago
Excuse me, I gave the wrong URL. It actually came from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n, but those also seems to have been built late enough.
(Assignee)

Comment 267

9 years ago
(In reply to comment #266)
> Excuse me, I gave the wrong URL. It actually came from
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n,
> but those also seems to have been built late enough.

Hrm, I wonder if l10n interferes with it somehow. I'm sadly not yet really familiar with how product branches are built. Can you try a plain nightly?
(In reply to comment #254)
> Yeah, it's Vista with NVIDIA GeForce 9200M GS.
> Guess I'm doomed to miss all the bugs. No black holes, slow loading pages,
> wrong fonts etc. Sigh.

Are you sure you have the Platform Update for Windows Vista installed?  This is the update that enables Direct2D and DirectWrinte on Windows Vista.  More info here: http://support.microsoft.com/kb/971644.
(Assignee)

Comment 269

9 years ago
We've discovered that some add-ons can interfere with D2D too, by prematurely initializing gfxPlatform. See bug 549090.

Comment 270

9 years ago
Aside from Adblock, Stylish apparently does the same.

About minimum requirements:
I've used the previous build quite extensively on a Geforce Go 7900GS and didn't find any unexpected behaviour (not in the current nightly either).

If we really want to enable this selectively, I'd suggest building a database of OS versions/chipsets/driver versions and let nightly testers report on specific combinations, instead of just setting a minimum requirement.

Comment 271

9 years ago
Sorry to infofrommozilla@justmail.de and hickendorffbas@gmail.com : I just a Bugzilla collision and accidentally removed them from the CC list. I've re-added them.

Comment 272

9 years ago
Based on comment 269 and 270, I'll try it in safe mode as soon as I get
the time to start up my PC again, as I have both the AdBlock Plus and Stylish
extensions installed and enabled.

I am also considering to try a "plain nightly", but I don't want to mess around
too much with the computer, as it's a laptop I need for work.

(In reply to comment #268)
> Are you sure you have the Platform Update for Windows Vista installed?  This is
> the update that enables Direct2D and DirectWrinte on Windows Vista.  More info
> here: http://support.microsoft.com/kb/971644.

If I have installed all the available updates for Windows Vista, including
optional ones, have I received that one?

Updated

9 years ago
Depends on: 549097

Updated

9 years ago
Depends on: 549102

Comment 273

9 years ago
Starting Minefield in Safe mode solved it for me, the changes and bugs are now clearly visible. And, speaking of bugs, I am getting some really odd ones on my local projects. For example, where the letter is suppose to be a "T", I am getting an "R", and in another place, I get a ")" instead of a ( ), space. I use @font-face in that case, if it matters.
(In reply to comment #267)
> (In reply to comment #266)
> > Excuse me, I gave the wrong URL. It actually came from
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n,
> > but those also seems to have been built late enough.
> 
> Hrm, I wonder if l10n interferes with it somehow. I'm sadly not yet really
> familiar with how product branches are built. Can you try a plain nightly?

FYI WFM with the latest L10n nightly build
Mozilla/5.0 (Windows; U; Windows NT 6.1; sk; rv:1.9.3a2pre) Gecko/20100226 Minefield/3.7a2pre
I've created tracking bugs for enabling direct2d (bug 549116) and directwrite
(bug 549115) by default.
(In reply to comment #272)
> (In reply to comment #268)
> > Are you sure you have the Platform Update for Windows Vista installed?  This is
> > the update that enables Direct2D and DirectWrinte on Windows Vista.  More info
> > here: http://support.microsoft.com/kb/971644.
> 
> If I have installed all the available updates for Windows Vista, including
> optional ones, have I received that one?

Yes, this should be OK.
Depends on: 549146

Comment 277

9 years ago
Stylish doesn't break d2d, I have it running.  ABP does however.  I also noticed that when adding add-ons you may have to restart FF twice in order for d2d to work.

Also there are a couple of themes that have separate optional extensions to customize the themes more fully.  One is Chromifox Extreme which uses Chromifox Companion extension.  The other is Strata40 which uses Stratabuddy extension.  Both of these extensions (not the themes) break d2d.

Comment 278

9 years ago
Doesn't take much to break things with d2d/dw enabled.  Changing an extensions options in Extension Manager can do it.  Uninstalling an addon can break it. 

Also images don't always download all the way.  You can have just a few lines of the picture, half of the picture, etc.  

Oh yeah, the letters w and v display badly.

But I must say that when it's working right it sure speeds things up.
I can't find any screenshots anywhere of any site taken with Minefield and D2D/DW enabled, so I can't compare it to what my browser is looking like. However, I am not seeing any issues with the w and the v being displayed badly. This is how mozilla.org looks on my end with D2D/DW enabled: http://i.imgur.com/Nyx8q.jpg.
We need a wiki page with instructions on how to collect data to report for D2D/D3D bugs: hardware and driver versions at least, I guess. Plus instructions on how to attach that information to bugs in a way that lets us categorize bugs based on that data. I'm afraid we're going to drown in a sea of "D2D doesn't work on my system" bugs.

Comment 281

9 years ago
(In reply to comment #277)
> Stylish doesn't break d2d, I have it running.  ABP does however.  I also
> noticed that when adding add-ons you may have to restart FF twice in order for
> d2d to work.

You're right: The restart-twice-necessary screwed up my testing.

Updated

9 years ago
Depends on: 549190

Comment 282

9 years ago
The w and v visual problems seems to depend on the site you are on.  Here they look fine.  I guess it's font related.  Here is a screenshot of the letters looking bad.  I got them from a post of mine at forums.mozillazine.org

Screenshot at http://img265.yfrog.com/img265/7688/capturetg.jpg

Comment 283

9 years ago
Gary and others: Please file new bugs as this bug has been resolved and all related bugs caused by the patches in this bug should be filed separately.
(Assignee)

Comment 284

9 years ago
(In reply to comment #283)
> Gary and others: Please file new bugs as this bug has been resolved and all
> related bugs caused by the patches in this bug should be filed separately.

There's tracking bugs for enabling DirectWrite and D2D by default:
Bug 549115 and bug 549116

Comment 285

9 years ago
For bug purposes what should be filed in DW and in D2D??  To me they are the same.
(In reply to comment #285)
> For bug purposes what should be filed in DW and in D2D??  To me they are the
> same.

Text drawing problems/oddities == DW, other drawing problems == D2D.

Updated

9 years ago
Blocks: 549269
No longer blocks: 549269
Depends on: 549269

Updated

9 years ago
Depends on: 549272