Closed Bug 177805 (pixels) Opened 22 years ago Closed 18 years ago

Fix the use of units in Gecko

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha3

People

(Reporter: roc, Assigned: sharparrow1)

References

(Depends on 2 open bugs, Blocks 4 open bugs,
URL
)

Details

Attachments

(2 files, 16 obsolete files)

ems testcase
222 bytes, text/html
Details
Patch that was checked in
747.02 KB, patch
Details | Diff | Splinter Review 
Currently we have a proliferation of units and conversions in Gecko which are
very difficult to understand, which induce a lot of conversion code (some
special-cased for specific situations like Print Preview), and which appear to
be leading to rounding errors (hard to tell because things are too complex).

Here's my proposal for fixing the situation:
1) Define 'layout units' to be exactly 1/100th of a CSS pixel.
2) Use these layout units everywhere.
3) Scale these units to device units inside GFX. Do not perform any device unit
scaling outside GFX.
4) In Widget, provide a scale factor that maps layout units to window system pixels.
5) In GFX, provide a scale factor that maps physical lengths to layout units.
This scale factor is device specific. This scale factor would be used only in
the style system to convert lengths in physical units into layout units.

dbaron wrote:
> For example, in an ideal system, I might want to have
> three constants for:
> 1. converting reference pixels to app units (which would be points for
>    printing or fractions of pixels for display)
> 2. converting physical lengths to app units, and
> 3. an overall scale.

Rougly, I'm suggesting fixing 1. to some constant (say, 1/100), 2. corresponds
to my 5), and 3. is achieved by varying 3) and 5).

I have to go now, I'll attach some rationale later.
I suppose for the sake of consistency with existing code we should call layout
units 'app units'.

After some more thought I think the magic number should be 1/720 of a CSS pixel.

I'm assuming that we bend the CSS rules a bit and choose the size of a CSS pixel
to make sure that the ratio of screen pixels to CSS pixels is always an integer
(i.e., 1 for most of today's screens, 2 for screens like IBM's 200dpi LCD panel,
maybe more later). If we don't do this it would be a disaster for the clarity of
many Web pages, no matter what else we do. Then for ratios 1, 2, 3, 4, 5 and 6
we get an integral number of app units per screen pixel. Beyond that it probably
doesn't matter.

By fixing app units to 1/720 of a CSS pixel, we guarantee that if the author
specifies lengths in the form NNN.Npx, all constraints will be computed exactly.
For example, a horizontal stack of boxes of specified lengths will always fit
perfectly into a box whose width is specified to be the sum of the box lengths.

1/720 of a CSS pixel is nominally about .4 microns, so we get good fidelity even
for very high resolution output devices. But we still get 1.4 million pixels in
30 bits, which is enough for a > 100,000 line document with reasonable font height.
Inorder to fix the roundoff problems.. keep in mind:

Anytime something is draw onto the screen it has to either 
     a.)  placement on the screen has to use a layout unit that is coicident with
          the screens pixel.. roundoff of this unit would not help.  
   or
     b.)  The error from this placement has to be kept around for any   
          calculations relative to this object so it can be calculated correctly.

I have created examples in another bug (63336), to show this is the problem
behind our roundoff problems.  A unit that keeps an error might be the best..
like floating or a fixed unit (16 bits decimal, 16 bits fractional)

I totally agree with the cleanup of our conversion and unit cleanup, we have
duplicate units, duplicate calls, calls that have poliferated outside of GFX,
calls with confusing names.  I am in for helping out on this one.



dcone wrote:
> like floating or a fixed unit (16 bits decimal, 16 bits fractional)

You are aware that output devices like printers can easily hit coordinates
beyond 65536, right ?
It was just an example.. we could use a 24 decimal and 8 fractional or floating
or double.  I really did not give thought to the exact unit to use.  But it
would only have to take care of our layout/application needs like twips does,
the conversion to the device units would be kept in the 32 bit longs.
In this framework, the print device tells us the physical size of a CSS pixel,
which we use to size physical lengths such as points and inches. Print preview,
shrink to fit and print scaling use the same number for physical size of a CSS
pixel, but also specify a scale factor to GFX to shrink the printed output to
fit on the screen or the page. This may require modifying GFX to make sure the
GFX transform is properly applied to fonts (and everything else that gets rendered).
> placement on the screen has to use a layout unit that is coicident with
> the screens pixel.. roundoff of this unit would not help.

Authors can specify lengths that aren't integer multiples of screen pixels, so
we can't entirely avoid rounding for screen display. The best we can do is make
sure that the conversion from app units to screen pixels is nice and clean.
That's what my proposal aims for: CSS pixels (the most important author unit)
are an integer multiple of app units, and screen pixels are also an integer
multiple of app units.

1/720 of a CSS pixel is essentially a fixed point unit. There's no need for the
denominator to be a power of two, and in fact there are some advantages to not
making it a power of two.

I don't think we should use floating point for layout. That makes errors
inevitable, such as in the case I suggested about making boxes fit exactly
inside their parent.

We may or may not want to use floating point for GFX scaling to device
coordinates (we currently do). We might eliminate some problems by using integer
arithmetic there too.
Maybe 1/60 would be a better number. It can still represent NNN.Npx exactly, and
it still handles screens with 1, 2, 3, 4, 5 and 6 screen pixels per CSS pixel.
It also gives us over a million lines of 10px text.
Should this be done now, or as part of Gecko 2 (and thus blocking bug 168884)?
Gecko 2 is just Gecko with bugs fixed.
QA Contact: gerardok → moied
The reporter in bug 178330 is asking for the same thing as dcone --- round every
measurement to device pixels before we do layout. This has a number of
unpleasant properties but if it's what the people want, I guess we could do it.
Blocks: 178330
No longer blocks: 63336
I suspect it's not what the CSS working group wants.  In particular, I've heard
mention that any size larger than zero should result in non-adjacency (but not
necessarily visibility).
More WG guidance would be most welcome.
Kevin and I were discussing this yesterday and Kevin brought up the impact this
would have on printing. Don do you have any thoughts on this?

We must all keep in mind that printing is not a GFX/transformation matrix issue.
It is a layout issue. We must always reflow for printing. 

It's not that I like twips, but when layout is in twips it is completely
disconnected from any device or the definition of what a pixel may or may not be
on any particular device.

Also, fixing this, may fix 153080. I do see an interesting issue with font sizes
and how/when they are defined with either Points or Pixels. Currently the
conversion from Points to Twips is a device independent conversion (thus
contributing to the problems in 153080) and Font sizes defined in Pixels, of
coarse, is a device dependent conversion.

So currently the PresShell defines a bunch of default faults in Pixels but their
"real" size in twips cannot be calculated until there is a Device. So there is a
bunch of really screwed up code throughout making assumptions about where the
current font size in twips came from (again Bug 153080).
> We must all keep in mind that printing is not a GFX/transformation matrix
> issue.

Understood. However, I believe that print preview is, or at least should be, a
process where we layout exactly as if we were printing, and then apply a GFX
transformation to make the result fit on the screen. Correct me if I'm wrong.

By the way, I hope you realize that CSS pixels are intended to be *somewhat*
device independent. Read http://www.w3.org/TR/CSS21/syndata.html#length-units if
you haven't already. Especially the example "In the second image, an area of 1px
by 1px is covered by a single dot in a low-resolution device (a computer
screen), while the same area is covered by 16 dots in a higher resolution device
(such as a 400 dpi laser printer)." So CSS pixels are device dependent in that a
CSS pixel should be an integral number of device pixels, but they are device
independent in that we set that integer to get a CSS pixel as close to 1/96in as
we can. [Hmm, the spec makes it sound like we should have a "how far away from
the computer are you sitting" preference :-).]

> but when layout is in twips it is completely
> disconnected from any device or the definition of what a pixel may or may not
> be on any particular device.

Yeah, and that's actually a problem because it creates rounding errors visible
on the screen unless the user selects certain "good" DPI values which make
pixels an integral number of twips. We must guarantee that a screen pixel is an
integral number of app units. That connection has to be there or we lose. That
is true no matter how we fix bug 63336.

[This constraint only applies when device pixels are bigger than app units. When
device pixels are smaller than app units, as would sometimes be the case for
printing under my proposal, I don't think we have to worry about the app
unit/device pixel ratio, because the only visible rounding effects should be
one-device-pixel changes in the sizes of objects depending on where they're
positioned, and those changes would be very hard to spot.]

Note that a major advantage of moving away from twips is that we let the user
use arbitrary DPI values. The DPI value will only be used to convert physical
lengths (pt, in, etc) to app units.

I agree with you that it would be nice to have app units be as device
independent "as possible" given the above constraints. That's why I'm proposing
using fractions of CSS pixels as the basis.

> So currently the PresShell defines a bunch of default faults in Pixels but
> their "real" size in twips cannot be calculated until there is a Device.

I think my proposal fixes this. A font size specified in CSS pixels maps to the
same number of app units no matter what the device.
OK, so this all sounds good, now what do we all think about scaling? 

For discussions sake, let's let "scaling" be a reflow issue and "zooming" be a
magnification issue (gfx transformation matrix). Note that "text zooming" (TZ)is
a reflow issue. The fonts sizes are scaled (and nothing else is) and then reflow
occurrs.

Currently we turn off zooming for printing/PP for a couple of reasons:

1) Scaling was not designed into the current system so we co-opt the Canonical
Pixel Scale for doing all the scaling and currently Point sized fonts are not
scaled (Bug 153080)

2) If a user has TZ set AND scaling set what exactly does that mean? Do you just
apply both? This could confuse end users as to the size of their final printout
or what they see in PP.


Scaling is where the user enters "N%" into the Page Setup box?

What exactly is that intended to do? If the user specifies 50% scaling, should a
"1 in" CSS box be printed as 1 in or as 0.5 in? I assume the latter. If so, then
here's how we do scaling by N%:
-- GFX returns a page width/height multiplied by 100/N
-- GFX returns the same DPI value that it would without scaling
-- Layout as normal for printing. Layout knows nothing about scaling.
-- GFX applies a transformation to scale everything by N/100
In other words, we just pretend the paper is bigger, and then we scale down the
resulting output to fit onto the real paper.

This is totally orthogonal to text zoom. If you scale by 50% then all text and
images shrink by 50%. If you scale by 50% and text zoom by 2x then text ends up
the same size but all images (and other measured lengths) shrink by 50%.

If you do print preview with scaling in effect:
-- GFX returns a page width/height multiplied by 100/N
-- GFX returns the same DPI value that it would without scaling
-- Layout as normal for printing. Layout knows nothing about scaling OR print
preview (as far as units are concerned)
-- GFX applies a transformation to scale everything by N/100
So far, just like above...
-- GFX applies a transformation to scale everything by CSS pixels for window
width/CSS pixels for page width

Note that in both of these stacks there is a final transformation from CSS
pixels to device pixels, and these transformations are different for the printer
and the screen.

Print Preview is totally WYSIWYG. Everything is laid out and rendered just as it
will be printed, with a pure geometric transformation at the end to make the
results fit in the window.
I don't quite understand how printing and screen units interact right now... but
would the proposal on the table also address the issue we have where changing
the screen dpi changes the size of the printed output?
Yes. Screen DPI would have no effect on printed output. Well, almost. Screen DPI
would be used in two ways:
-- GFX uses the screen DPI to figure out how many screen pixels to use for each
CSS pixel, when rendering to the screen
-- GFX uses the screen DPI (and the above decision) to compute how many CSS
pixels there are per inch on the screen (this only affects documents being laid
out for the screen, and does not affect layout for printing or print preview)

The first decision affects printing of widgets whose dimensions are taken from
the OS. E.g., if the OS tells us that a scrollbar is N pixels wide, then we need
to convert that to CSS pixels for layout, and that conversion depends on the
screen DPI. E.g. for a 200dpi screen we might choose to have 2 screen pixels per
CSS pixel. So the OS says a scrollbar is 30 pixels wide, but that's really only
15 CSS pixels, and that's what we use in layout, for both screen and printing.

Other than that, screen DPI should have no effect on printing.
Ok... so the size of images in printed output will still depend on the screen
DPI then, for intrinsically sized images....
No, they won't. An intrinsically sized image of XxY pixels will be laid out with
a size of exactly X x Y CSS pixels --- NOT screen pixels. Yes, this means for a
200dpi screen most images will be scaled by 2x in each direction, but this is
actually what you want.
This is not an theoretical question, by the way. Some people where I work have
IBM T221 displays, which are 200dpi. They look fabulous but you go blind surfing
the Web with today's browsers. This proposal will largely fix that.
Ah, cool!  I was wondering whether we'd treat image pixels as CSS pixels...

That may make perf a bit worse on high-dpi displays (since we will be scaling
continuously) but I think it's well worth it.
It will perf worse if CSS pixels != device pixels.

It also will look worse, since the scaling algorithms are ... simplistic.  See
bug 98971, bug 73322, and bug 4821
Depends on: 98971
Even the dumbest scaling algorithm will work fine, since we'll guarantee that
theere is always an integral number of device pixels per CSS pixel. E.g., on a
200dpi screen, a normal image will display just as it would on a 100dpi screen,
using a 2x2 block of device pixels for each image pixel. The only other simple
alternative is simply to display everything at half the size, which I believe is
worse.

[Of course the best alternative would be to implement a better scaling algorithm
so the image on a 200dpi screen would be the same size as the image on a 100dpi
screen, but would actually look better. But that's beyond the scope of the
issues here.]
Well, roc's proposal gets my vote. Let's roll! :-)
Component: Layout → Layout: Misc Code
QA Contact: moied → ian
Component: Layout: Misc Code → Layout
sorry for the component change.. didn't mean to.

How is this going to impact viewing image on a site for the normal user?  What I
mean is if you have a 72x72 pixel image on a normal user's screen, we don't want
to be resizing it at all.. it would be a huge perf hit if we were.
Component: Layout → Layout: Misc Code
We won't be resizing anything for 'normal' users. I suggest using the following
equation, which seems to more or less follow from the W3C definition of a CSS pixel:

devicePixelsPerCSSPixel = min(1, round(screenDPI/96))

So devicePixelsPerCSSPixel is 1 until screenDPI >= 144.

There will come a day when normal users have screens with that kind of DPI ---
but by then hopefully things will have sped up enough we won't be worrying about
the scaling.
Doing this will not fix any round off problem.. we all agree to that?  What are
the other reasons to fix the units in Gecko, I don't understand what we gain
using a CSS pixel or some scale of that as an internal unit.  

The internal unit we decide to use has to be a unit that maps well to all
devices and any unit that is used on that device.. thats the only requirment for
an interal unit.. correct?

Also.. permormance issues.. and continually scaling.. BIG TIME HIT IN SPEED..
not something you can discount.   

Other problems we have.. and need to be addressed.. setting anything in pixel
units (like fonts), what does this mean when we specify a pixel unit for a scren
then print out.  This causes alot of conversion problems..because the printer
has a completely different pixel size.  We have to be careful about what we are
trying to solve and the bounds we want to stay in.  We have been messing around
with page layout issues and will get into this even deeper with things coming
down the road... so I think its really important to really understand all the
issues.  

I feel like I've already thoroughly explained the requirements for units and how
my proposal meets them. I'm not sure how to make it clearer. But I'll try to
summarize.

Layout has to work in units that are some integer fraction of CSS pixels. If we
don't, then we incur roundoff errors in layout that mean, for example, 7 boxes
of 7 pixels wide each might not fit exactly inside a 49-pixel wide box. [Right
now layout DOES work in units that are some integer fraction of CSS pixels, but
only because we force users to use a screen DPI that makes the twips to pixels
ratio an integer. That sucks.] There are various other reason why doing layout
in something related to CSS pixels makes sense: e.g. a CSS pixel is somewhat
resolution independent, yet it will always have a nice relationship to screen
pixels. Also, rods can tell you how we have font prefs specified in CSS pixels,
and it's convenient to let them be device-independent.

There are some roundoff issues that this does not solve --- namely, roundoff
when we try to display a document that has features with finer resolution than
the output device can display. Those problems are covered in bug 63336. Even if
we decide to address those issues by (for example) rounding up the sizes of
laid-out things to whole pixels, that can be done independently of the proposal
here.

> Also.. permormance issues.. and continually scaling..

I think I just explained in comment #27 that this would only happen for devices
with high DPI (say, >= 144DPI), which is very few screens today. And when you do
have a screen like that, scaling is better than going blind, trust me.

> what does this mean when we specify a pixel unit for a scren then print out.

I already addressed this at length. This proposal actually improves the printing
situation a lot. Please remember that we're talking about CSS pixels and a CSS
pixel is roughly the same size on the screen as on the printer.
Why don't we plan a meeting to discuss this. 
I have many questions, so does Rod.  I can study and read in the meantime to
understand what this all means to the GFX engine and what it would take to
accomplish this for the advantages we want to get out of it.
I have written up something a little more logically organized:
http://ocallahan.org/mozilla/units.html
If there's a meeting about this, please dial me in.
A CSS pixel 1/96 of an inch.. right (lets say normal arms and normal eye angle).
 I am getting stuck on this point.. I am assuming a CSS pixel is a virtual
device at 96 dpi.. of certain angle and viewing distance.


I will set up a meeting next week.. chim in if you want to be included.  So far
its me, roc,rod,ian and I will get kevin to attend.
A CSS pixel is not always 1/96 of an inch.

From CSS 2.1:
> Pixel units are relative to the resolution of the viewing device, i.e., most
> often a computer display. If the pixel density of the output device is very
> different from that of a typical computer display, the user agent should
> rescale pixel values.

What this means is that for typical computer displays a CSS pixel should be
exactly a screen pixel, even if the screen is not 96dpi. So if you have a 72dpi
screen a CSS pixel will be 1/72 of an inch; if you have a 120dpi screen, a CSS
pixel will be 1/120 of an inch. If you have a 200dpi screen my proposal suggests
we should make a CSS pixel be 1/100 of an inch (as close to 96dpi as we can get
while still making a CSS pixel be an integral number of screen pixels).

A phone meeting sounds fine.
I'd also want to be included in this meeting.

Also note that this bug is somewhat independent of moving to correct CSS pixels,
in that fixing this doesn't really require that we do so.  So please don't get
hung up on the scaling issues here.  Nevertheless, moving to correct CSS pixels
is something that we should do.
If the meeting is Tues or Thurs, I would like to dial in.
In comment 35, I probably should have said "moving to correct CSS pixels for
screen displays", since we already scale CSS pixels for printing (although we
may not do it quite right).
> Also note that this bug is somewhat independent of moving to correct CSS pixels

That's true, but moving to correct CSS pixels for high-DPI screens imposes an
additional constraint on the choice of N, so I think it's worth at least taking
that into account now.
How about on Tuesday afternoon, early for us East-coast-ers? 1pm West coast time?
Could we give times in UTC or explicitly UTC-08 or -05 or whatever the relevant
time zone is?

I'm in UTC+00 at the moment.
Component: Layout: Misc Code → Layout: Block & Inline
Version: Trunk → 1.0 Branch
Here's my summary of yesterday's meeting:

This general approach was viewed favourably by everyone. In particular the idea
that fonts should be scaled by the current GFX transform seemed to be accepted.

There is one major unanswered question, and it needs some deep thought. That is
the question of whether layout should align boxes to device pixels, or whether
all such rounding should be deferred to GFX. Looking at it another way, the
question is how to render a fragment like this on a normal screen:

<div style="width:6px; height:9px; background:black;">
  <img style="width:1.5px; height:9px; background:red;">
  <img style="width:1.5px; height:9px; background:green;">
  <img style="width:1.5px; height:9px; background:blue;">
  <img style="width:1.5px; height:9px; background:yellow;">
</div>

If we round the IMG widths to device pixels during layout, then either an IMG
will wrap to the next line or some black will be visible at the end of the line.
Arguably, either is unacceptable.

On the other hand, if we do the rounding during GFX rendering, then some of the
IMGs will be rendered 1px wide and some 2px wide. One could also argue that that
is unacceptable.

So the question is, which unacceptable behaviour are we going to choose? :-)
I think rounding/conversion from app units to device units (pixels) should be
done at the very last possible moment.  Who knows when some code might want to
add 3 "app units" to x-pos.  Converting at layout restricts gfx from ever
modifying position/size accurately.
My opinion is that on the whole, we should try to preserve layout even if it
means strange rendering. If we have a layout which involves fractional pixels
then we simply can't render it 100% accurately; I'd rather be off by a pixel
here and there than to risk a gross layout change, such as causing something to
wrap that shouldn't, or causing something to be uncovered that was supposed to
be completely covered.

However, I'm open to using specific hacks to get things aligned on screen pixels
when we have leeway in certain situations. For example, no matter what layout
units we use, we probably should lie about the size of 1mm or 0.1in to make it
an integral number of screen pixels. Similarly karnaze has table hacks that try
to align %-sized table columns on pixel boundaries. I think that's fine and
good, but it works only because of this particular situation: authors shouldn't
rely on getting an exact length from a percentage, and even if we give them a
slightly incorrect length we know (because it's in a table) that we won't be
screwing up the overall layout too much. When an author specifies an exact
length I don't think we have quite the same freedom.
I agree that we should try to honour fractional pixels as closely as possible.
In the example you give, I would expect, at page zoom = 200%, to see:

   +--+--+--+--+--+--+--+--+--+--+--+--+
   |  red   | green  |  blue  | yellow |
   +--+--+--+--+--+--+--+--+--+--+--+--+

...and at 100%, either:

   +-----+-----+-----+-----+-----+-----+
   |    red    |green|   blue    |yello|
   +-----+-----+-----+-----+-----+-----+

...or:

   +-----+-----+-----+-----+-----+-----+
   | red |   green   |blue |  yellow   |
   +-----+-----+-----+-----+-----+-----+

...depending on our exact implementation. I think it is vital that zooming (or
changing device, which should be exactly equivalent) should not radically change
the layout, which is the only possibility if we don't do it this way.

> For example, no matter what layout units we use, we probably should lie about 
> the size of 1mm or 0.1in to make it an integral number of screen pixels.

Well we certainly can't do both at the same time. If we say 1mm is an integral
number of pixels, that forces our resolution to an integral multiple of 25.4dpi,
a kind of restriction from which we are trying to get away. Furthermore if we
say that 1mm is an integral number of pixels then 0.1in must be an integral
multiple of 2.54 pixels, which is most definitely not an integral number of
pixels itself. Similarly, 1pt can only be an integral number of pixels if the
resolution is an integral multiple of 72dpi.


On another note, it was mentioned at one point that rounding at rendering time
rather than at layout can cause odd-looking effects while scrolling, as the
pixels fall on different boundaries. This can be easily solved by doing all the
maths for rendering relative to the canvas origin rather than the rendering
surface origin, and simply rendering the right part to the screen.

Note that an OpenGL GFX would probably automatically have OpenGL do anti-
aliasing for the pixels that contain two colours...
Alias: pixels
> Well we certainly can't do both at the same time.

We can cheat by lying about the length of a millimetre and an inch. Suppose I
have a 92dpi screen. Then 1mm is 3.54 dots. Let's lie and say it's 4 dots. We've
made 1mm 10% bigger than the true millimetre. (Of course we then make 1cm be 40
dots.)

Similarly, 0.1in is 9.2 dots, but we can lie and make it 9 dots. Then 1in is 90
dots so it's 2% smaller than a true inch. Furthermore 1in would be 22.5mm
instead of 25.4.

I wouldn't recommend messing around with points; just leave them at 1/72 of 1in.

The only way I can see this causing trouble is if a) someone holds a ruler up to
the screen or b) an author mixes mm/cm with in in the same page and expects
things to line up. I expect b) to be confined to psychotic test case writers :-).

> scrolling

Actually this is most easily solved by making sure all scroll offsets are always
screen-pixel-aligned. That's what we currently do anyway.

> OpenGL GFX

It's not always possible to anti-alias everything in OpenGL, and even on an
anti-aliased device we want things to be pixel-aligned as much as possible, or
it will look bad. But yes it would help us render stuff that can't be aligned,
like my example.
Using the origin boundary will not solve this.  We use lots of relative
measurments, widths, etc, etc.  So easily solved.. I disagree.  As soon as you
start drawing a path.. for example..and have an ending point.. and the next path
depends on this ending point for its starting point.. all bets are off.

Anti Aliasing in OpenGL.. is very expensive.. and not a gimme.  Anti-aliasing is
not built into OpenGL.. although there is mechanism with there buffers to Anti
alias.. but its not part of OpenGL.. path drawing or line drawing.  OpenGL is a
3D drawing enviorment.. so any 2D drawing is an after thought.  You can not even
draw concave polygons in OpenGL unless you supply your own routines to rasterize
them.

The problems specificallly related to scrolling are easily solved. Other
problems are not.

OpenGL is WAY offtopic for this bug, but anti-aliased line and polygon
rasterization is built into the spec (GL_SMOOTH mode), although implementations
may or may not do it well. Also, you can draw concave polygons without
rasterizing them yourself, using some clever tricks with the stencil buffer, XOR
mode, and the "fan" triangle-drawing mode. But let's just forget Hixie ever
mentioned OpenGL :-).
Re: comment 41 and ff.
Interestingly, this problem is reminiscent to what happens with scalable fonts,
when trying to fit a glyph sequence in a given, bounded, space. What happens
with fonts is that those glyphs get scaled differently (a.k.a. non-linear
scaling / hinting). With fonts however, the font designer _knows_ the list of
characters encoded in their fonts, and hardcode a built-in mechanism that knows
how to particularly weight 'm' vs 'a' vs etc, so that the overall result is
almost always pleasant/uniform on any given string taken from the font under
consideration.

With abritray boxes however, say B1, B2, sometimes one would want the
compensation to go in B1, sometimes one want it to go in B2, in a consistent
way, (e.g., in a vertical stacking within table cells where the user may want a
neat alignment of the right edge of B1 elements for example, or in the case of
horizontal staking, a neat line up of the stacked baseline). It might be in this
area that the trickiest problems could arise.

You might this article useful:
http://msdn.microsoft.com/archive/default.asp?url=/archive/en-us/dnargdi/html/msdn_ttfonts.asp

[BTW, TeX uses a hundredth (1/100) of the wavelength of the light as its
internal unit. Hence while rendering might look jagged at places on the screen
(eg with xdvi), when printed, round-off errors fall below the visible threshold
and that's why it still looks so nice even with today's high resolution printers.]
> TeX uses a hundredth (1/100) of the wavelength of the light

TeX has a luxury we do not -- it's rendering on something about the size of a
printed page (give or take an order of magnitude).  It only needs to keep track
of what's going on on that one page.  We're often forced to render on a canvas
that's hundreds of thousands of lines tall...  And the worst part is that with,
eg, relative positioning things at the bottom of that canvas can affect what's
going on at the top, so we can't just render one part of it.  :(
Re: comment 45, I thought inaccurate computation of real-world units for
arbitrary DPI settings was one of the problems this was intended to solve.
> > TeX uses a hundredth (1/100) of the wavelength of the light
>
> TeX has a luxury we do not

Yep, I know that. The point is that even with its paranoic precision Knuth-like
unit an all this compute time that makes some jealous (batch vs
web-interactive), integer round-off errors arise, and are all the more crude on
the screen (as noted in comment 46). So if folks do indeed agree that roc's
proposal solves other issues in a foward-looking manner, then well, let's have a
try, as a solve-all system is impossible. But using a fractional unit (as TeX or
comment 2) might probably lay a much solid foundation for the proposal.
> The only way I can see this causing trouble is if a) someone holds a ruler up to
> the screen or b) an author mixes mm/cm with in in the same page and expects
> things to line up. I expect b) to be confined to psychotic test case writers :-)

Psychotic test case writers are the people who will be proclaiming loudly that
Mozilla is non-standards compliant if 2.54cm != 25.4mm != 1in != 72pt.

Also, if we have a dpi setting then we really should make an effort to make it
useful -- accurate lengths are a part of that.
Come to think of it, there's no need to worry that much about absolute units. If
we do render them in apparently uneven ways, it'll only help convince people to
avoid them. We're already trying to convince authors to only use 'em' and '%'
anyway (most of them use 'px', grrr).
You don't want authors to specify line widths in ems, do you? Ugh.

Most likely 0.1em will not be an integral number of screen pixels, in which case
we are probably going to see rounding error artifacts in em-based layouts unless
we do something *really* evil.
Line widths should typically be specified by using 'em' or '%' units, yes. How
else can you get a scalable multi-media stylesheet?

I don't understand why you would get any artifacts. Could you attach a
non-pathological testcase which demonstrates what you mean?
Why can't we just round accurately but consistently, always scroll by integer
numbers of pixels, and use units that are fractions of a pixel so that any
integer number of pixels can be expressed in our units?
Attached file ems testcase — 
Here's a testcase which uses ems and will have problems when 0.1em is not an
integral number of screen pixels; some of the boxes will have different sizes.

Right now we do even worse, because we sometimes show one-pixel wide lines
separating some of the boxes, but that's just an inconsistent rounding bug
somewhere, not a design problem. If we rounded consistently we would never see
this particular problem.
> How else can you get a scalable multi-media stylesheet?

You can use px in a scalable multimedia stylesheet. It is roughly 1/72 of an
inch. On a low resolution, antialiased device you SHOULD use px for line widths
because it's the only way to guarantee you don't get crap-looking antialiased
1.5 pixel wide vertical and horizontal lines. And as far as I know, px is also
the only way to guarantee you get boxes displayed with consistent sizes as
measured in screen pixels.

> Why can't we just round accurately but consistently, always scroll by integer
> numbers of pixels, and use units that are fractions of a pixel so that any
> integer number of pixels can be expressed in our units?

I think we all agree that we should do these things at least. The question is
whether we should do additional work to try to lay out objects at screen pixel
boundaries. dcone says "yes". I say "sometimes, but we must be sneaky to avoid
breaking layouts". Are you saying "no"?

This question could be deferred, but it's worth considering now because one
particularly convenient way to force things to lay out at screen pixel
boundaries would be to make layout units be screen pixels. Personally I think
this would break layouts too much (e.g. by causing inappropriate line wrapping),
and so I think we should just choose to make app units be 1/60 of a CSS pixel
and later see where we can judiciously add more round-to-screen-pixels logic to
layout.
> some of the boxes will have different sizes

Why is that a problem?
>> How else can you get a scalable multi-media stylesheet?
> You can use px in a scalable multimedia stylesheet.

Sorry, I should have been clearer. By "scalable" I meant one which honours the
user's font size settings.

The only valid use of pixels in stylesheets, IMHO, is for 1px borders.
>> some of the boxes will have different sizes
> Why is that a problem?

Because it looks ugly (and people file bugs about it).

> By "scalable" I meant one which honours the
> user's font size settings.

Why should line widths depend on the user's font size settings?

It sounds like you want font size settings to act as a general "zoom everything"
control. I would rather implement a seperate "zoom everything" control in the UA
for that. That would handle intrinsically sized images and everything, without
burdening authors.
I'm not saying everything should obey the user's font size, and indeed I
explicitly mentioned 1px borders as a good use for pixel units. But solid pages
should be built with 'em's and '%'s so that they adapt to the user's environment
and interact well with user stylesheets.


>>> some of the boxes will have different sizes
>> Why is that a problem?
> Because it looks ugly (and people file bugs about it).

I would much, _much_ rather have 0.7em sometimes being 10 pixels and sometimes
11 pixels rather than have strange, unpredictable hacks that make some or all of
0.1em, 0.1cm, and 0.1in be integral numbers of pixels, just to ensure that a
subset of sites happen to render with square squares.

How about the site that has 0.15em? Or the site that uses 0.5em of 0.5em? Or the
site that uses percentages of a prime number of pixels? We can always come up
with cases that will end up as non-integral. If authors want to ensure that they
get exactly equal results, then they should indeed use pixels (and hope the user
hasn't zoomed in or out). But in the majority of cases, we should just lay out
documents at the subpixel level, and then render onto the pixel grid using well
defined rules (e.g. a pixel takes the color of just inside its top right corner)
and consistent rounding.
Here's a testcase for what I mentioned in comment 44:
   http://www.hixie.ch/tests/adhoc/css/units/007.html

I think we can easily fix Gecko to render that correctly at all zoom levels,
even 100%. (We actually don't do that badly at 800% in print preview at the moment.)
Blocks: 106083
Just a little comment from a user POV (I've just read the whole bug history)
Whatever unit you choose, please make it based on em not millimeters, inches or
some hardwired "96dpi" rule.

The days of the 96dpi screen are numbered. They'll die with crts. Processors are
still getting faster and that's not just for games.

(if you want some sort of rounding to make old devices speedier just make it an
option - the user is clearly the one who can decide on a quality vs speed trade-off)

I happen to use true 100dpi displays. Native apps know it and act accordingly. I
can assure you wherever a browser (or another app) decides I shall use a 96dpi
display I see it at once. So don't give me **** about 96=100 (or even 144=96). A
user can feel it. I don't want to scale things in my mind or with my eyes just
to save some CPU time. I *paid* (or my employer did) for a powerfull system to
do things for me. Last I've checked it was a bargain compared to an eye
operation some time in the future.

Books are still the best reading media because they do *not* satisfy themselves
with good enough (if you doubt this just try to xerox a few pages slightly
changing the scaling between each of them, and then try to read it). The
internet is still mostly a written media. That means accurate font scaling
must/should be paramount in a browser, and hence internal units be em-based.
Blocks: 102755
Blocks: 230573
Blocks: 90579
One comment on http://ocallahan.org/mozilla/units.html : I think I prefer N=20
over N=60, since I don't think thirds and sixths matter very much since it's not
possible to specify them accurately in decimal.  (The advantage is that it gives
us three times the space to avoid nscoord overflow.)
Also, regarding making fonts obey the current transform -- currently
nsTransform2D seems capable of representing any affine transformation (i.e., the
composition of a linear transformation and a transposition) in 2 dimensional
space.  Most (all?) of its callers assume, however, that it represents only the
composition of a scaling or a transformation (i.e., they assume it's two
independent transformations on the two axes).  We should perhaps make the code
less general since we don't need it.  However, I could imagine that we might
want the ability to scale the two dimensions by different amounts (e.g.,
different dpi in each dimension, although this shouldn't matter for printing,
I'd think), and it might be hard to make fonts obey that (although they might
just come out "right" anyway).

Should we throw out the ability of the transform to scale the dimensions by
different amounts (we don't support that now anyway, although X does)?  If not,
how do we make fonts obey the current transform?
Never mind comment #66 -- I was forgetting about the issue of screens with 3
device pixels per CSS pixel.
Consider SVG ... everywhere we currently read the transform matrix is probably a
bug. We need general affine transforms.

IMHO font renderers that can't yet handle general affine transforms should just
assert that the matrix is one they can handle.
If we want general affine transforms we need to fix all the rendering context
methods that do anything with rects or ellipses -- i.e., anything that uses
nsTransform2D::TransformCoord(nscoord *aX, nscoord *aY, nscoord *aWidth, nscoord
*aHeight), since any caller of that method is inherently broken.
ahhh, I thought you were talking about callers from layout.

Sure, callers within Gfx make all sorts of assumptions. But SVG-friendly Gfx
implementations require tons of work anyway.

As far as this bug is concerned we just need to change the font API to specify
font sizes in app units.
Version: 1.0 Branch → Trunk
Blocks: 270106
Blocks: 271274
Attached patch WIP (obsolete) — — Splinter Review 
A start.  Still some issues with this.	The biggest ones are Print Preview is
messed up, page margins are messed up, header/footer printing is broken, and
there are some rounding problems, like text selectom moves text.  This is also
Windows-only at the moment because of the GFX changes.
Any chance you could summarize what it is that you're doing?
The basic idea is that I changed nsIDeviceContext so that instead of providing
methods for converting between device pixels and twips, it provides methods for
converting between device pixels and CSS pixels, CSS pixels and application
units, and CSS Pixels and Twips.  Then, I changed most of the consumers
converting between device pixels and twips to converting between app units and
CSS pixels.

I think this is the right idea.  This patch isn't really complete at the moment
(not everything is converted, some things were incorrectly converted, and a lot
of things are incorrectly labeled).  Note that one I've fixed the patch, a
200dpi screen will display anything sized in CSS pixels using two device pixel
per CSS pixel.

I currently have it set up so the App Unit per CSS pixel ratio is a variable,
but it only gets initialized to 60 and never actually changes (and probably will
never need to change during runtime).  Should I define it to be a fixed number?
 If so, should I get rid of the device context calls to retrieve this number?
Hey, this looks VERY cool. A few comments...

Don't use #if 0. Just remove the code.

> I currently have it set up so the App Unit per CSS pixel ratio is a variable,
> but it only gets initialized to 60 and never actually changes.

That's good. Let's leave it like that.

-  nsMargin(nscoord aLeft,  nscoord aTop,
-           nscoord aRight, nscoord aBottom) {left = aLeft; top = aTop;
+  nsMargin(PRInt32 aLeft,  PRInt32 aTop,
+           PRInt32 aRight, PRInt32 aBottom) {left = aLeft; top = aTop;

Please don't do this and don't remove the nsIntMargin stuff! nscoord will change
to float eventually. See bug 265084.

I've never liked this two-step process where we obtain a scaling factor and then
do some transformation. Couldn't we just have methods on nsPresContext, perhaps
like this?

  // rounds down
  PRInt32 AppUnitsToIntCSSPixels(nscoord aV);
  nsIntPoint AppUnitsToIntCSSPixels(nsPoint aPt);

etc? What do you think?
I think the scaling factor thing was done for performance reasons, although I
can't see it being much of a win, considering how infrequently it's used.  I'll
go ahead and do that.
It should be relatively easy to make sure that the conversion functions are
fully inlined, so performance is not an issue.
Attached patch WIP 2 (obsolete) — — Splinter Review 
Still has issues: most noticeable are that images in printing are tiny and some
text moves around when it is selected or hovered over.	Large DPI screens are
still mostly broken.  I've been trying to fix device/css pixel issues, but
haven't been making much progress; I must still have some of the device pixel
and CSS pixel callers mixed up.  I probably won't be able to do much on this
during the week, but if anyone wants to help, that would be nice.
Attachment #177341 - Attachment is obsolete: true
I moved my units page into the wiki, it's now here:
http://wiki.mozilla.org/Mozilla2:Units
Feel free to edit.

One thing I rembembered during the conversion is that it's really important that
the CSS pixels to app units value be a constant across the entire app. So I
think we should make CSSPixelsToAppUnits static in nsIDeviceContext. In fact I
think for  clarity's sake we should just have these in nsIDeviceContext:

  static PRInt32 AppUnitsPerCSSPixel() { return ...; }
  PRInt32 AppUnitsPerDevPixel() { return ...; }
  PRInt32 CSSPixelsPerInch() { return ...; }

Everything else can be derived from these.
Okay, say we have only those 3 methods on nsIDeviceContext.  Then say some code
needs to convert from App Units to Device Pixels.  Should every use be written
out explicitly, or should there be some utility functions somewhere, say in
nsIDeviceContext.h, like the following?
inline PRInt32 NSAppUnitsToDevPixelsFloor(nsIDeviceContext* aDeviceContext,
nscoord aAppUnits)
{
    return NSToIntFloor((float)aAppUnits /
                        aDeviceContext->AppUnitsPerDevPixel());
}
I think the main helpers should be on the prescontext. There could be static
helpers in nsIDeviceContext for code that doesn't have a prescontext, but that's
hardly anyone.
Should I convert callers to use the form
nsPresContext::CSSPixelsToAppUnits(lengthInPixels) rather than
aPresContext->CSSPixelsToAppUnits(lengthInPixels)?
Attached patch WIP 3 (obsolete) — — Splinter Review 
Working patch with suggested interfaces.  This still needs more work, primarly
fixing printing and print preview and tracking down a few bugs.  However, it is
ready for comments at the interface level.

Interface-wise, I think I need to change AppUnitsPerDevPixel on the PresContext
to be a float.	I don't see any other alternative that allows for a full zoom
implementation in which device pixel rounding works.  I know that a
non-integral number of app units per dev pixel has been a source of bugs, but I
don't see an alternative.  Note that I'm not suggesting changing the device
context method.  Also, the difference would not be noticible with this patch
alone.
Attachment #178209 - Attachment is obsolete: true
The attachment appears to not be plain text...

AppUnitsToDevicePixels shouldn't be affected by a general zoom transformation,
since zooming should not affect layout. IMHO we should do a general zoom just by
changing the active transformation in the device context when we render. This
doesn't currently work because our current Gfx implementations don't apply the
current transformation's scaling to fonts, but we'll fix this in the new Gfx
implementation based on Cairo.
Attached patch WIP 3 again (obsolete) — — Splinter Review 
I posted the other one gzipped; I guess I shouldn't do that.

The case I'm wondering about concerns, for example, the table code that rounds
to pixels.  Suppose zoom is set to 140%.  If AppPixelsPerDevicePixel doesn't
change for that document, we'll round to multiples of 60 app units when the
number of app units in a device pixel is really 300/7~=42.86.  Without
accounting for that somehow, any code that does pixel rounding will be
worthless.
Attachment #179363 - Attachment is obsolete: true
Zooming shouldn't change the layout, so the table code is just going to have to
do the right thing assuming no zoom. The results won't be quite right for
non-integral scaling factors, but I think we can live with that.
Well, zooming should change the layout, since zooming in effectively shrinks the
window width.
That really depends on what you want the zooming feature to be. If you want to
generally surf around with everthing scaled up or down, sure, we'd adjust the
window width so content fitted into the window. If you just want to temporarily
zoom in to see some part of a page in more detail, I claim you don't want to
change the layout.
There's significant user demand for the former (since it would fix many of the
problems with text zoom).
Okay. But I think if the user chooses a scaling factor so that the number of
device pixels per CSS px is not an integer, the results are going to be bad in
general. So I think for general-purpose zooming while browsing you don't want to
offer all possible scale factors.
I'm trying to figure out where to store the scaling factor for printing.  I'm
thinking that I should store it on the view and have the view manager transform
the rendering context in nsViewManager::Display and nsViewManager::Refresh. 
Does that sound like the best way to do it?
In theory it's now a generic scaling factor, not a printing-specific thing, right?

We could store it on the PresShell and make it accessible via nsIViewObserver.
How about that?
(In reply to comment #91)
> Okay. But I think if the user chooses a scaling factor so that the number of
> device pixels per CSS px is not an integer, the results are going to be bad in
> general. So I think for general-purpose zooming while browsing you don't want 
> to offer all possible scale factors.

'1px' zoomed by 150% should render exactly the same as '1.5px' zoomed by 100%. I
don't see the problem here.
Pages whose elements have dimensions like 1.5px tend to look kinda bad.
Basically, some of the boxes will be 2px and others will be 1px depending on
where they happened to be laid out. Sure, we can enable all possible zoom
factors if you don't care about that.
So long as we're doing layout with sub-device-pixel resolution, rounding at
rendering time is fine, IMHO. So yes, I think we should not worry about
preventing users from being exposed to this problem.
Attached patch WIP 4 (obsolete) — — Splinter Review 
I've kinda gotten scaling working, but I'm having some issues with clipping. 
At this point I'm looking for some comments on whether the way I'm storing zoom
on the view make sense and some idea what I'm doing wrong with clipping.
Attachment #179406 - Attachment is obsolete: true
It's looking really good.

(In reply to comment #97)
> Created an attachment (id=180349) [edit]
> I've kinda gotten scaling working, but I'm having some issues with clipping. 
> At this point I'm looking for some comments on whether the way I'm storing
> zoom on the view make sense

I think doing per-view or even per-view-manager scaling is not a good idea,
since it becomes unclear how to handle subdocuments with different scale
factors. How about making the scale just be a property on the root view manager?
It doesn't need to affect view coordinate setting at all. It just gets applied
to the rendering context before painting and to incoming event handling
coordinates. I think that would simplify your code quite a bit.

> and some idea what I'm doing wrong with clipping.

No idea, sorry. What sort of problems are you seeing?
Never mind about the clipping thing; it was just a typo.

I initially tried something similar to putting the zoom in the view manager, but
I was having a lot of trouble with stopping the things that weren't supposed to
scale from scaling (like the scrollbar in Print Preview and the footer).  I
settled on my current solution because it accurately scales only the things that
need to be scaled.  I'm actually a bit confused why what I'm doing isn't
working.  Is there not a single view tree for documents and their subdocuments?

If I were to put the zoom on the view manager, the PageContentFrame would need
to be the root view in a view manager.  Are you proposing I add that?
(In reply to comment #99)
> Never mind about the clipping thing; it was just a typo.
> 
> I initially tried something similar to putting the zoom in the view manager,
> but I was having a lot of trouble with stopping the things that weren't
> supposed to scale from scaling (like the scrollbar in Print Preview and the
> footer).  I settled on my current solution because it accurately scales only
> the things that need to be scaled.  I'm actually a bit confused why what I'm
> doing isn't working.  Is there not a single view tree for documents and their
> subdocuments?

There is.

> If I were to put the zoom on the view manager, the PageContentFrame would need
> to be the root view in a view manager.  Are you proposing I add that?

No. I hadn't thought of the stuff that should not be scaled. That's really
annoying...

If we have to have a per-view scale factor, then at least call it
nsIView::SetScale. Are you sure you need to adjust the coordinates in
nsViewManager::CreateView, nsViewManager::MoveViewTo, and
nsViewManager::ResizeView? I would have thought not, although you would have to
rescale coordinates when translating them up to the parent's coordinate space.
Am I missing something or is the scale factor not currently being applied to
painting and event handling?

That's all a big hairy ball to get right. I need to think about whether there's
a better way.
I'm tempted to suggest that we impose a uniform scale over the entire view
manager tree and deal with print preview by explicitly scaling up the scrollbar
and page borders.
> If we have to have a per-view scale factor, then at least call it
> nsIView::SetScale.

Fine.  I actually changed it to nsIViewManager::SetViewScale to keep with the
precedent.

> Are you sure you need to adjust the coordinates in
> nsViewManager::CreateView, nsViewManager::MoveViewTo, and
> nsViewManager::ResizeView? I would have thought not, although you would have to
> rescale coordinates when translating them up to the parent's coordinate space.

We actually have a choice here: should views be stored in scaled or unscaled app
units?  Different pieces of code in layout have different assumptions.  I chose
unscaled as easier to get working well enough for printing (since events and
scrolling don't matter on a printed page), but full zoom on the screen would
require changes either way.

(In reply to comment #101)
> I'm tempted to suggest that we impose a uniform scale over the entire view
> manager tree and deal with print preview by explicitly scaling up the scrollbar
> and page borders.

The only way to scale the scrollbar is to somehow tell the view controlling it
that paticular scrollbars should not be scaled.  Having to mark which stuff
needs to be scaled like that explicitly requires adding code for every unscaled
element, which is almost the same thing; I think it's just shifting around the
code to add.  Also, with the scaling factor on the view, how the code works is
very clear: everything outside a certain view is not scaled, everything inside is.
(In reply to comment #102)
> > Are you sure you need to adjust the coordinates in
> > nsViewManager::CreateView, nsViewManager::MoveViewTo, and
> > nsViewManager::ResizeView? I would have thought not, although you would have
> > to rescale coordinates when translating them up to the parent's coordinate
> > space.
> 
> We actually have a choice here: should views be stored in scaled or unscaled
> app units?  Different pieces of code in layout have different assumptions.  I
> chose unscaled as easier to get working well enough for printing (since
> events and scrolling don't matter on a printed page), but full zoom on the
> screen would require changes either way.

I think unscaled is the correct way, anyway.

> (In reply to comment #101)
> > I'm tempted to suggest that we impose a uniform scale over the entire view
> > manager tree and deal with print preview by explicitly scaling up the
> > scrollbar and page borders.
> 
> The only way to scale the scrollbar is to somehow tell the view controlling it
> that paticular scrollbars should not be scaled.  Having to mark which stuff
> needs to be scaled like that explicitly requires adding code for every
> unscaled element, which is almost the same thing; I think it's just shifting
> around the code to add.  Also, with the scaling factor on the view, how the
> code works is very clear: everything outside a certain view is not scaled,
> everything inside is.

Yeah. I think we'll just have to go with the per-view scale and make it work. We
can simplify the task by enforcing a requirement that only views associated with
certain kinds of frames are allowed to have a scale. That will limit the places
where we have to consider coordinate transformations taking scale into account.
For now, that would only be nsPageContentFrame.
(In reply to comment #100)
> (In reply to comment #99)
> > the things that need to be scaled.  I'm actually a bit confused why what I'm
> > doing isn't working.  Is there not a single view tree for documents and their
> > subdocuments?
> 
> There is.

Actually, that's only true for screen rendering at the moment (see
http://lxr.mozilla.org/seamonkey/source/layout/printing/nsPrintEngine.cpp#2678).
 It's not really relevant to this bug, but I don't want anyone who reads this
bug to get confused like I did.
Attached patch WIP 5 (obsolete) — — Splinter Review 
This patch still has a couple visible issues.  One is certain elements shifting
around by a pixel when they are repainted.  I'm not sure what's causing this. 
The other is that the platform-specific font measurement code doesn't account
for the scaling factor.

Because of the work that would be required to port the complete fix to all the
GFX backends, I guess there's not much point to trying to get this in before
Cairo is enabled by default.  Therefore, I guess I'll just keep this patch
up-to-date in my tree and let checking it in wait until Cairo is the default on
all platforms.

In case anyone's interested, with this patch, 200dpi screen rendering is
usable, but there are some noticable issues.  I think I'll end up disabling
having more than one device pixel per CSS pixel on screens when this gets
checked in and deal with the issues in another bug.
Attachment #180349 - Attachment is obsolete: true
Depends on: 97861
Attached patch WIP 6 (obsolete) — — Splinter Review 
Updated to trunk with some minor fixes for 200dpi displays.  Besides the
rounding and font scaling bugs, the only noticable issues with this patch are
that on 200dpi displays, SVG doesn't scale and the autocomplete popup is in the
wrong spot.

The changes I've made to fix screen/device pixel bugs are kludgy, but they show
where issues exist (look for conversions from device pixels to app units to CSS
pixels).  The biggest offender is improper use of DOM events, which have
coordinates in CSS pixels, not device pixels.  XUL really shouldn't be using
the DOM coordinates to handle mouse events.
Attachment #182141 - Attachment is obsolete: true
Assignee: roc → sharparrow1
Blocks: 305558
I am interested in seeing bug 4821 implemented. Could someone explain whether fixing this bug (177805) automatically fixes bug 4821 or whether additional work is necessary? If the latter, what work would remain? Finally, I don't see a target milestone. Is there any desire to see this released soon (by the people working on this)? The last comment included a patch, but that was six months ago.
Depends on: 323934
Attached patch WIP redone for trunk (obsolete) — — Splinter Review 
I figured I might as well post this in case anyone wants to look at it.  Overall, it's working well for regular screen rendering, but it needs work in some areas and a some cleanup.

Windows only at the moment, but it should be very easy to get working on other platforms that use thebes.

I've been seeing some rounding problems testing at 200dpi: the most noticable is that images are rendering oddly, for example the toolbars and Google image searches.

Other problems at 200dpi: There are problems rendering the borders of textboxes.  I still need to fix tooltips.

I haven't reimplemented scaling for printing yet.  I'm getting a little confused about how to do it using display lists.  Should I create my own kind of display item?  And how do I hook it up to scale the content?
Attachment #185921 - Attachment is obsolete: true
We should think about how this work plays with the resolution independence in Mac OS X. See <http://webkit.opendarwin.org/blog/?p=55>.
+  static const PRInt32 mAppUnitsPerCSSPixel = 60;

Should be gAppUnitsPerCSSPixel

+  PRInt32 mAppUnitsPerDevPixel;
+  PRInt32 mAppUnitsPerInch;

I think all 3 of these, and the return types of associated methods, should be nscoords. nscoord is the type of appunits.
(In reply to comment #110)
> We should think about how this work plays with the resolution independence in
> Mac OS X. See <http://webkit.opendarwin.org/blog/?p=55>.

It's pretty much exactly compatible with what hyatt was talking about there.
Actually I think we should get rid of mAppUnitsPerCSSPixel and just make it a build-time constant --- still sourced ultimately from nsIDeviceContext::AppUnitsPerCSSPixel(), as you have it now (so we can make it dynamic if we later want to).
I'll make those changes to nsIDeviceContext.

roc, could you give me some pointers about doing scaling with display lists?  I think I need a new type of item, but I'm not sure how to go about that.
> I haven't reimplemented scaling for printing yet.  I'm getting a little
> confused about how to do it using display lists.  Should I create my own kind
> of display item?

I think the best approach would be to create an nsDisplayItem subtype for scaled page contents which is actually a leaf display item. When asked to paint, it would add a scale transformation to the incoming rendering context, build a display list for the page contents, paint it, and then pop the transformation. This avoids confusion because the page contents are really in a different coordinate system to the outside, so we shouldn't put them in the same display list structure. nsSVGForeignObjectFrame does something a bit like this. This would require more work if we want events to be able to target content in print preview, but we don't (currently). If we ever do need events, we follow/extend the way foreignobject works.
Attached patch WIP v8 (obsolete) — — Splinter Review 
Okay, this takes care of most of the issues.

One issue remaining is themed rendering and printing.  Textboxes get printed with one-device-pixel thick lines, which are many times thinner than the borders on the screen.

The other big issue is the painting of images.  Toolbar images at 200dpi are drawing with pixels from adjacent icons.  At 200dpi, images are drawing with blurry edges.  When printing, images are sometimes outlined on the bottom and right edges.  I think some of these problems show up without my patch, though; try scaling a Google image search using a foerignobject (https://bugzilla.mozilla.org/attachment.cgi?id=224965) in a regular build and you'll get similar results.

I'm not sure if the MathML changes are right, particularly with the sub and sup frames; the behavior won't change, but I'm not sure it was right originally.  Someone familiar with that code should check I'm doing the right thing.

I fixed up the table code in a very minimal way.  I'm not sure if I should worry about cleaning it up now, since this patch is already so big.

I guess this patch will have some time to sit until Thebes progresses more (a couple months?).
Attachment #226459 - Attachment is obsolete: true
(In reply to comment #116)
> One issue remaining is themed rendering and printing.  Textboxes get printed
> with one-device-pixel thick lines, which are many times thinner than the
> borders on the screen.

Hmm. This sounds like a bug in the Windows theme-drawing code --- theme drawing  should be obeying the current Thebes context. Does the same problem occur in foreignobject-zoomed content?

> The other big issue is the painting of images.  Toolbar images at 200dpi are
> drawing with pixels from adjacent icons.  At 200dpi, images are drawing with
> blurry edges.  When printing, images are sometimes outlined on the bottom and
> right edges.  I think some of these problems show up without my patch, though;
> try scaling a Google image search using a foerignobject
> (https://bugzilla.mozilla.org/attachment.cgi?id=224965) in a regular build and
> you'll get similar results.

OK, interesting. Not your bugs then.

> I guess this patch will have some time to sit until Thebes progresses more (a
> couple months?).

Yes. We'll need to enable Thebes on all platforms and then get to the point where we don't want to support non-Thebes builds, not even for comparision purposes.
(In reply to comment #117)
> (In reply to comment #116)
> > One issue remaining is themed rendering and printing.  Textboxes get printed
> > with one-device-pixel thick lines, which are many times thinner than the
> > borders on the screen.
> 
> Hmm. This sounds like a bug in the Windows theme-drawing code --- theme drawing
>  should be obeying the current Thebes context. Does the same problem occur in
> foreignobject-zoomed content?
> 

No, it doesn't, although foreignobject scaling gives weird results for scrollbars (weird in a different way from the result of my patch).  The theme code isn't aware of CSS pixels, and I think that that is the issue.  I'll have to look at the theme code a bit more carefully.
Blocks: 256880
Flags: blocking1.9?
Blocks: 353860
+    mAppUnitsPerDevPixel = AppUnitsPerCSSPixel() / PR_MAX(1, dpi / 96);

I think this should round dpi/96. So e.g. 150dpi should lead to pixel-doubling. But I could be wrong, I'm not 100% sure.
(In reply to comment #116)
> The other big issue is the painting of images.  Toolbar images at 200dpi are
> drawing with pixels from adjacent icons.  At 200dpi, images are drawing with
> blurry edges.

This sounds like bug 324698 -- basically Cairo's image sampling is braindead and needs to be fixed.  Making pixman do the right thing at image edges is on my todo list, I've just been very afraid to delve into that code.
(In reply to comment #119)
> +    mAppUnitsPerDevPixel = AppUnitsPerCSSPixel() / PR_MAX(1, dpi / 96);
> 
> I think this should round dpi/96. So e.g. 150dpi should lead to pixel-doubling.
> But I could be wrong, I'm not 100% sure.

So, more like PR_MAX(1, (dpi + 48) / 96))?  Of course, we can always tweak it even after the checkin.

If we're going to support arbitrary scaling, though, we really shouldn't be using that number directly anywhere, becuase any assumptions about it will break when a page is scaled (the print stuff deals with it, but the screen is a lot more complicated.)
Status: NEW → ASSIGNED
(In reply to comment #121)
> So, more like PR_MAX(1, (dpi + 48) / 96))?  Of course, we can always tweak it
> even after the checkin.

Yeah.

> If we're going to support arbitrary scaling, though, we really shouldn't be
> using that number directly anywhere, becuase any assumptions about it will
> break when a page is scaled (the print stuff deals with it, but the screen is a
> lot more complicated.)

What are you worried about here, specifically?
(In reply to comment #122)
> > If we're going to support arbitrary scaling, though, we really shouldn't be
> > using that number directly anywhere, becuase any assumptions about it will
> > break when a page is scaled (the print stuff deals with it, but the screen is a
> > lot more complicated.)
> 
> What are you worried about here, specifically?

Nothing important. Stuff that's being taken care of by other work going on.   Eventually it really shouldn't matter what it's set to because there will be other ways to scale everything.
Flags: blocking1.9? → blocking1.9+
Depends on: 322938
No longer depends on: 323934
While this may not come as much of a shock given that the last patch was made in June, WIP v8 fails miserably when attempting to apply it to the current trunk.
After reflow branch lands (next week or two), someone will need to remerge this, which will be tons of work, but then we can review and land it!
(In reply to comment #125)
> After reflow branch lands (next week or two), someone will need to remerge
> this, which will be tons of work, but then we can review and land it!

My take:

1. What about BeOS and OS/2?  A bit of research seems to show that BeOS is out of the picture because trunk doesn't build on their version of GCC.  For OS/2, I've asked in the newsgroup, and hopefully I'll know soon.

2. Are cairo-gtk2 and cairo-cocoa really stable enough to completely break non-Cairo builds?  (Or rather, will they be a month from now?)  On Windows, everything seems to be working well enough, but I'm not sure whether the builds are good enough to dogfood for other platforms.

3. It might be a good idea to fix print preview before checking this in, so that it is possible to test print preview; on the other hand, it might be easier to fix once the canonical pixel scale stuff is gone.  (The issue is that the AltDevice stuff is currently stubbed out in thebes gfx.)

4. (In reply to a private email.)  Testing isn't really necessary at the moment, because the patch will have to be redone to apply to the trunk.  Once the patch is updated, though, I'd really appreciate testing, since this patch touches a lot of code.

5. I think I'll update the patch sometime in December, unless someone else wants to do it.  Unfortunately, most of it has to be done by hand; having the older patch as a reference helps, of course, but the codebase constantly changes, and each conversion really needs to be evaluated individually because people haven't been very good about using the right conversions (the distinction between scaled pixels and device pixels is only visible when printing at the moment).

6. Is the reflow branch really landing in a week or two?  Cool :)  (Nobody's mentioned it online.)
1. I believe OS/2 currently builds. However it does lag behind at times and I don't think it should block landing a patch of this magnitude and importance, so feel free to break it if that helps.

2. cairo-gtk2 and cairo-cocoa are on by default now. We'll ship them or die trying. We are ready to break non-cairo builds, or we will be soon enough.

3. It would be good if we could have print preview working in this patch. How hard is that?

4. I'm sure our QA supermen (and women) are standing by!

5. Makes sense. You definitely only want to update once more before landing.

6. The trunk is closed and being prepped for 1.9a1, which should happen next week. I believe we're landing reflow branch ASAP after that.
Well 6. isn't quite accurate, but basically yes, we're landing the reflow branch soon.
(In reply to comment #127)
> 3. It would be good if we could have print preview working in this patch. How
> hard is that?

Print preview isn't actually broken; it's just not using all the right data.  What's currently broken is that we're not getting the right page size and the font measurement is wrong. Hmm, actually it should be really easy to fix those, since they only occur in a few places.  Print preview font rendering would still be broken, but that never worked properly (bug 109970).
(In reply to comment #127)
> 1. I believe OS/2 currently builds. However it does lag behind at times and I
> don't think it should block landing a patch of this magnitude and importance,
> so feel free to break it if that helps.

Robert, I agree. But I still wanted to point out that it was really nice of Eli to post in the OS/2 newsgroup and point us to this issue. The manpower on the OS/2 port is very limited so any hints like these are always very welcome!
Attached patch WIP v9 (obsolete) — — Splinter Review 
Okay, new version updated to trunk.  I think this is ready for testing, if anyone is willing.  I'd especially appreciate testing with high-resolution displays and printing, since issues are much more likely to come up there (print preview acts sort of strange, but that's true on the trunk as well).

Known issues: there are some rounding errors rendering images and some minor theming issues when device pixels per CSS pixel is more than 1.  Also, SVG is not rendering at the right size.

If anyone wants to test changing the dpi, you can use the hidden preference "layout.css.dpi".  You don't have to restart the browser, but changing it messes up currently open windows.

I'm going to do a bit more cleanup before asking for review.
Attachment #227012 - Attachment is obsolete: true
One issue I'm seeing with the patch is that there is a border missing for the inactive tabs in Firefox, see:
http://wargers.org/mozilla/bug177805_units/177805_units_tabbar.htm
I'm seeing white stripes on images in print preview, when they are out of view and then scroll them into view:
http://wargers.org/mozilla/bug177805_units/white_stripes.jpg
(image url: http://stuff.mit.edu/people/ec_mok/www/pics/landscape/lake1.jpg )
(In reply to comment #132)
> One issue I'm seeing with the patch is that there is a border missing for the
> inactive tabs in Firefox, see:
> http://wargers.org/mozilla/bug177805_units/177805_units_tabbar.htm

Plus:
- the active tab is 1px higher (see left and right borders),
- the active tab's label (icon and text) is situated 1px lower.
(In reply to comment #134)
> (In reply to comment #132)
> > One issue I'm seeing with the patch is that there is a border missing for the
> > inactive tabs in Firefox, see:
> > http://wargers.org/mozilla/bug177805_units/177805_units_tabbar.htm
> 
> Plus:
> - the active tab is 1px higher (see left and right borders),
> - the active tab's label (icon and text) is situated 1px lower.

Not quite right. Both tabs are shifted 1px down, which means that the inactive tab's label and both close buttons are shifted 1px up.
With the patch, iframes without width or height set, seem to get no width:
http://wargers.org/mozilla/bug177805_units/177805iframe_no_width.htm
Text is moving with the patch when selecting some text on this page:
http://wargers.org/mozilla/bug177805_units/177805_moving_when_selecting_letter_spacing.htm
I found this on http://tom-eric.info/
(In reply to comment #136)
> With the patch, iframes without width or height set, seem to get no width:
> http://wargers.org/mozilla/bug177805_units/177805iframe_no_width.htm

Fixed.

(In reply to comment #137)
> Text is moving with the patch when selecting some text on this page:
> http://wargers.org/mozilla/bug177805_units/177805_moving_when_selecting_letter_spacing.htm
> I found this on http://tom-eric.info/

Fixed.

Thanks for the testing!  I'm still working on fixing the other issues, and I'll post a new patch when I'm done.
We're hot to try this on the OLPC machine which has a 200dpi screen.  When you update the patch again, we'll try it out.
Assuming SVG isn't totally broken, can somebody with the patch applied please
test if it fixes bug 364638? There should be 6 black squares in the testcase in
attachment 250406 [details], not just 4.
Mac OS X, the patch applied well, but I get a build error almost immediately:

/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp: In member function 'nsresult nsThebesDeviceContext::SetDPI(PRInt32)':
/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:139: warning: unused variable 'OSVal'
/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp: In member function 'virtual nsresult nsThebesDeviceContext::Init(void*)':
/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:225: error: 'NSFloatPointsToTwips' was not declared in this scope
/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp: In member function 'virtual nsresult nsThebesDeviceContext::GetDeviceContextFor(nsIDeviceContextSpec*, nsIDeviceContext*&)':
/Users/phiw/cocoafox/mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:521: warning: unused variable 'rv'
make[6]: *** [nsThebesDeviceContext.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2
philippe: try replacing NSFloatPointsToTwips(x) with
NSToCoordRound(x * mAppUnitsPerInch / 72.0f);
Eli, I think we should introduce helpers for converting nsRects, nsMargins, and nsPoints in various directions. However, if you don't want to do it in this patch, then that's fine with me.
(In reply to comment #142)
> philippe: try replacing NSFloatPointsToTwips(x) with
> NSToCoordRound(x * mAppUnitsPerInch / 72.0f);
>

Thanks, this brought me further. Unfortunately, more breakage:
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsNativeThemeCocoa.cpp:286: error: 'class nsDerivedSafe<nsIDeviceContext>' has no member named 'TwipsToDevUnits'
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsNativeThemeCocoa.cpp:351: error: 'NSTwipsToIntPixels' was not declared in this scope
make[6]: *** [nsNativeThemeCocoa.o] Error 1
...

(but now I have to bail out, got some work to do.)
Looking forward to an updated patch.
 


I think you want

    float t2p = 1.0f/dctx->AppUnitsPerDevPixel();

and replace NSTwipsToIntPixels with NSAppUnitsToIntPixels.
(In reply to comment #140)
> Assuming SVG isn't totally broken, can somebody with the patch applied please
> test if it fixes bug 364638? There should be 6 black squares in the testcase in
> attachment 250406 [details], not just 4.

This isn't going to fix that bug.  The style system stores all lengths in app units, and this patch doesn't change that; it just changes what exactly app units refer to.
Attached patch WIP v10 (obsolete) — — Splinter Review 
Updated patch.  Problem with lines in print preview partially fixed, although image rendering really needs some work.  Problem with tabs not fixed; I'm not sure what's causing the problem.  

For 200dpi displays, there are some significant issues with "cracks" in web layouts (for example, www.mozilla.org).  SVG scaling isn't working; I'm not completely sure how it should work, though.  I also need to look into why the scrollbars look bad (at least on WinXP).
Attachment #250329 - Attachment is obsolete: true
(In reply to comment #145)
> I think you want
> 
>     float t2p = 1.0f/dctx->AppUnitsPerDevPixel();
> 
> and replace NSTwipsToIntPixels with NSAppUnitsToIntPixels.

Not right; NSAppUnitsToIntPixels takes integer app units per pixel.  (This is different from the old NSTwipsToIntPixels behavior, which took floating point pixels per app unit.)

I put in the Mac changes from the comments in the latest patch; I'm not sure if it compiles yet, though.
(In reply to comment #148)

> 
> I put in the Mac changes from the comments in the latest patch; I'm not sure if
> it compiles yet, though.
> 
Unfortunately, is still doesn't compile:
The error I get now:
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsDragService.mm: In function 'NSRect GetDragRect(nsIDOMNode*, nsIScriptableRegion*)':
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsDragService.mm:150: error: 'class nsPresContext' has no member named 'TwipsToPixels'
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsDragService.mm:153: error: 'NSTwipsToIntPixels' was not declared in this scope
make[6]: *** [nsDragService.o] Error 1


I think

    float t2p = frame->GetPresContext()->TwipsToPixels();

    nsRect screenOffset;                                
    screenOffset.MoveBy(NSTwipsToIntPixels(widgetOffset.x + viewOffset.x, t2p),
                        NSTwipsToIntPixels(widgetOffset.y + viewOffset.y, t2p));

    outRect.origin.x = (float)screenOffset.x;
    outRect.origin.y = (float)screenOffset.y + (float)NSTwipsToIntPixels(rect.height, t2p);
    outRect.size.width = (float)NSTwipsToIntPixels(rect.width, t2p);
    outRect.size.height = (float)NSTwipsToIntPixels(rect.height, t2p);

should be

    nsPresContext* presContext = frame->GetPresContext();

    nsRect screenOffset;                                
    screenOffset.MoveBy(presContext->AppUnitsToDevPixels(widgetOffset.x + viewOffset.x),
                        presContext->AppUnitsToDevPixels(widgetOffset.y + viewOffset.y));

    outRect.origin.x = (float)screenOffset.x;
    outRect.origin.y = (float)screenOffset.y + (float)presContext->AppUnitsToDevPixels(rect.height);
    outRect.size.width = (float)presContext->AppUnitsToDevPixels(rect.width);
    outRect.size.height = (float)presContext->AppUnitsToDevPixels(rect.height);

Doesn't compile on my Linux box: 
/home/user/soft/src/mozilla-test/mozilla/gfx/src/ps/nsDeviceContextPS.cpp: In member function `virtual nsresult nsDeviceContextPS::InitDeviceContextPS(nsIDeviceContext*, nsIDeviceContext*)':
/home/user/soft/src/mozilla-test/mozilla/gfx/src/ps/nsDeviceContextPS.cpp:206: error: `mTwipsToPixels' was not declared in this scope
/home/user/soft/src/mozilla-test/mozilla/gfx/src/ps/nsDeviceContextPS.cpp:206: error: `NSIntPointsToTwips' was not declared in this scope
...and so on.
After doing the edits suggested in comment 150 (thanks Roc), I got one step further. 
Now I get errors in widget/src/cocoa/nsChildView.mm:
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm: In function 'void -[ChildView updateHandScroll:](ChildView*, objc_selector*, NSEvent*)':
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm:1913: error: 'class nsIDeviceContext' has no member named 'DevUnitsToAppUnits'
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm:1915: error: 'NSIntPixelsToTwips' was not declared in this scope

(and there will possibly come more errors after that one ...)

(In reply to comment #151)
> Doesn't compile on my Linux box: 
> /home/user/soft/src/mozilla-test/mozilla/gfx/src/ps/nsDeviceContextPS.cpp: In

you shouldn't be building gfx/src/ps in a cairo build.  this patch won't work in non-cairo builds
(In reply to comment #153)

> you shouldn't be building gfx/src/ps in a cairo build.  this patch won't work
> in non-cairo builds
Oops, sorry. However, it still doesn't compile with ac_add_options --enable-default-toolkit=cairo-gtk2 in .mozconfig:

 src/thebes/nsThebesDeviceContext.cpp:247: error: `NSFloatPointsToTwips' was not declared in this scope

Compiles cleanly without the patch. But I'm not sure I'm doing everything right.
Attached patch WIP v11 (obsolete) — — Splinter Review 
Added in compile fixes for Mac and Linux; untested, though.
Attachment #250482 - Attachment is obsolete: true
WIP v11 on Mac:
1/ one file (widget/src/cocoa/nsNativeThemeCocoa.cpp) does not exist anymore, it is replaced by
widget/src/cocoa/nsNativeThemeCocoa.mm
(it is the same file, by contents, actually).

2/ I ran into the same build error as in my last comment (comment 152) :
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm: In function 'void -[ChildView updateHandScroll:](ChildView*, objc_selector*, NSEvent*)':
/Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm:1913: error: 'NSIntPixelsToAppUnts' was not declared in this scope

Just to test, I replaced 'NSIntPixelsToAppUnts' by 'NSAppUnitsToIntPixels' in that file (nsChildView.mm).
Wonder and surprise, the build **succeeded**.
---> ***don't trust me*** that this is the correct 'fix'. That was a -maybe very stupid- way to see if any more build errors would show up.

3/ I ran tested that build for a while. It works.
Noticed that text in .GIF files was quite blurred.
Didn't find any major layout breakage.
I did see some problems with irregular spacing between elements in a page in the vertical space. But those are possibly coming from Cairo bugs.

(In reply to comment #156)
> WIP v11 on Mac:
> 1/ one file (widget/src/cocoa/nsNativeThemeCocoa.cpp) does not exist anymore,
> it is replaced by
> widget/src/cocoa/nsNativeThemeCocoa.mm
> (it is the same file, by contents, actually).

Okay; I'll update to tip.

> 2/ I ran into the same build error as in my last comment (comment 152) :
> /Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm: In function 'void
> -[ChildView updateHandScroll:](ChildView*, objc_selector*, NSEvent*)':
> /Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm:1913: error:
> 'NSIntPixelsToAppUnts' was not declared in this scope

Oops.  That should be NSIntPixelsToAppUnits.  (Notice the missing i.)

> Just to test, I replaced 'NSIntPixelsToAppUnts' by 'NSAppUnitsToIntPixels' in
> that file (nsChildView.mm).
> Wonder and surprise, the build **succeeded**.
> ---> ***don't trust me*** that this is the correct 'fix'. That was a -maybe
> very stupid- way to see if any more build errors would show up.

That function basically does the opposite, so it might cause problems. (I'm not sure how critical that code is.)

> 3/ I ran tested that build for a while. It works.
> Noticed that text in .GIF files was quite blurred.

Blurred? What do you mean?  Is the UI getting scaled up?

> Didn't find any major layout breakage.
> I did see some problems with irregular spacing between elements in a page in
> the vertical space. But those are possibly coming from Cairo bugs.

Hmm, for the moment, I think I'm going to use the slightly relaxed standard of "is it usable" for running at 200dpi.  For standard dpi screens (where the UI isn't getting scaled up), though, anything short of pixel identical to unpatched  builds blocks checking this in.  I'm not sure which group you're in from your comments.
(In reply to comment #157)
> (In reply to comment #156)
 
> > 2/ I ran into the same build error as in my last comment (comment 152) :
> > /Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm: In function 'void
> > -[ChildView updateHandScroll:](ChildView*, objc_selector*, NSEvent*)':
> > /Users/phiw/cocoafox/mozilla/widget/src/cocoa/nsChildView.mm:1913: error:
> > 'NSIntPixelsToAppUnts' was not declared in this scope
> 
> Oops.  That should be NSIntPixelsToAppUnits.  (Notice the missing i.)

Oops... missed that. Corrected that here, and the build completed successfully.
(and while I was at it, I made a Camino build as well. No problems either.)

> > 3/ I ran tested that build for a while. It works.
> > Noticed that text in .GIF files was quite blurred.
> 
> Blurred? What do you mean?  Is the UI getting scaled up?

.gif files inserted as elements in an html page.
Here is an example: <http://dev.l-c-n.com/_b/guardian.png>
from <http://www.guardian.co.uk/worldlatest/>
The text is slightly blurred in the centre.
The impression is one of an image that has been slightly resized in the html (but they are not).
On a site under development, I have a gif file that is positioned exactly on top of a .gif background image. With this patch applied, there is a 1px offset. Resizing the window horizontally, and I can see the image slightly moving; sometimes the alignment on the left is correct, sometimes on the right. The look of it is that the foreground image has a 1px difference in size. Both foreground and background image have exactly the same (horizontal) size.

Here is an other example, from the Fx toolbar; there is a 1px difference in size between the images and the inputfields
<http://dev.l-c-n.com/_b/xul-fx.png>

> 
> > Didn't find any major layout breakage.
> > I did see some problems with irregular spacing between elements in a page in
> > the vertical space. But those are possibly coming from Cairo bugs.
> 
> Hmm, for the moment, I think I'm going to use the slightly relaxed standard of
> "is it usable" for running at 200dpi.  For standard dpi screens (where the UI
> isn't getting scaled up), though, anything short of pixel identical to
> unpatched  builds blocks checking this in.  I'm not sure which group you're in
> from your comments.
> 

Here is an example. Screenshot <http://dev.l-c-n.com/_b/geckogaps.png>
page: <http://dev.l-c-n.com/gecko/bugs/gaps/gaps-1.html>.
Zooming in or out gives different result.

All tests on a standard PowerBook 1.5Ghz, running OS X 10.4.8.



> -  view->ScrollTo(NSToCoordRound((aX/ratio)*context->PixelsToTwips() - portRect.width/2),
> -                 NSToCoordRound((aY/ratio)*context->PixelsToTwips() - portRect.height/2),
> +  view->ScrollTo(nsPresContext::CSSPixelsToAppUnits(aX/ratio) - portRect.width/2,
> +                 nsPresContext::CSSPixelsToAppUnits(aX/ratio) - portRect.height/2,

Typo: aX vs aY

> @@ -954,36 +954,31 @@ nsThebesRenderingContext::DrawImage(imgI
...
> -    nsIntRect pxDr;
> -    pxDr.x = NSToIntRound(FROM_TWIPS(twDestRect.x));
> -    pxDr.y = NSToIntRound(FROM_TWIPS(twDestRect.y));
> -    pxDr.width = NSToIntRound(FROM_TWIPS(twDestRect.width));
> -    pxDr.height = NSToIntRound(FROM_TWIPS(twDestRect.height));
> +
> +    nsIntRect pxDr(twDestRect);
> +    pxDr.ScaleRoundOut(1.0f / mP2A);

This adds 1 pixel to width and height for small (normal) integers because
60 * (1/60) > 1 when using floats and ScaleRoundOut() uses ceil():
http://lxr.mozilla.org/seamonkey/source/gfx/src/nsRect.cpp#183

The "gaps" testcase in comment #158 shows normal behavior if 1em doesn't happen
to translate to an even number of pixels (or the margin between h5's isn't 1em).

I'm seeing a few problems on linux:

* Tabs are shifted down (similar to comment #123)
* There's a line of screwed up drawing up in the title bar
* One of the rounded images in the minefield start page is drawing way too big

http://www.campd.org/bz/177805/gtk-unpatched.png vs.
http://www.campd.org/bz/177805/gtk-patched.png
Current progress: I haven't really been getting very far with this.  I've been spending a lot of time trying to figure out why the Fx tabs aren't drawing correctly, but I haven't made any progress.  Any help in figuring out the cause of that issue would be extremely helpful.
Maybe the following code in nsCSSRendering::PaintBackgroundWithSC shouldn't be disabled for Linux?

#if (!defined(XP_UNIX) && !defined(XP_BEOS)) || defined(XP_MACOSX)
  // Setup clipping so that rendering doesn't leak out of the computed
  // dirty rect
  aRenderingContext.PushState();
  aRenderingContext.SetClipRect(dirtyRect, nsClipCombine_kIntersect);
#endif

But I don't see how this could interfere with the units patch.

Just ran across a bug in WIP v11. At line 3025 we have

         if (offset.x > width || offset.y > height) {
-            p0.x = - floor(fmod(offset.x, gfxFloat(width)) + 0.5);
-            p0.y = - floor(fmod(offset.y, gfxFloat(height)) + 0.5);
+            p0.x = - floor(fmod(offset.x, gfxFloat(width) * scale) + 0.5);
+            p0.y = - floor(fmod(offset.y, gfxFloat(height) * scale) + 0.5);
         } else {

The if statement should be changed too, and compare offset to the scaled width and height.

I have made an updated patch, diffed against yesterday's tree:
http://wargers.org/mozilla/bug177805_units/all.diff
What I noticed with the patch is that images are sometimes 1 pixel higher/wider:
http://wargers.org/mozilla/bug177805_units/widthheight.htm
http://wargers.org/mozilla/bug177805_units/widthheight2.htm
I suspect this is the reason why the 'Go' and 'Search' button are 1 pixel too high and have a weird dent. I also suspect (or rather hope) that it is the issue for the missing border line for inactive tabs (comment 132).
Blocks: 346367
(In reply to comment #162)
> Current progress: I haven't really been getting very far with this.  I've been
> spending a lot of time trying to figure out why the Fx tabs aren't drawing
> correctly, but I haven't made any progress.  Any help in figuring out the cause
> of that issue would be extremely helpful.
> 

Okay, now I feel kind of silly; I just checked, and it turns out the tabs are srewed up in exactly the same way without my patch (at least on my computer).  I might still be introducing a regression, but at least now I'm convinced there are factors outside this patch messing up the tabs.

In my latest version, I've gotten rid of the GFX changes that were causing additional rounding problems in various other places.  This unfixes the gaps in zoomed content in print preview, but I think that's more of a fundamental problem with the image rendering APIs.

I'll post my latest version shortly.
Attached patch Patch v12 (obsolete) — — Splinter Review 
Current state.  I think I might have to mess with the sizing/painting for native widgets a bit more to get them to cooperate with printing correctly, although that's not a very high priority.

About the issue with the tabs: I'm pretty sure there is some issue, but there's also an issue relating to tabs and certain OS font sizes, which masks the rounding issue.
Attachment #250639 - Attachment is obsolete: true
I'll update this for the textrun landing...
> What I noticed with the patch is that images are sometimes 1 pixel
> higher/wider:
> http://wargers.org/mozilla/bug177805_units/widthheight.htm
> http://wargers.org/mozilla/bug177805_units/widthheight2.htm

I don't see these bugs on Mac, nor the related chrome bugs.
Attached patch Patch v13 (obsolete) — — Splinter Review 
Updated for textrun landing and other trunk changes.

I built and tested this on Linux and Mac. It seems to work very well, at least in non-scaling mode, for browsing regular sites. Print preview works fine in Linux. Mac printing may be slightly busted, but that's in flux right now anyway.

I think we can, and should, land this very soon; as soon as we open after alpha2 (i.e., next week) at the latest. There will no doubt be some little cosmetic regressions but I think we'll be fine as far as alpha-quality builds go.
Attachment #252251 - Attachment is obsolete: true
Obsoletes for v13?

I tried to made some fixes for gfx/src/gtk2....

It buildable, but does not work ;(

Eli could you look on my changes for gfx/src/gtk... and say what wrong there?
(In reply to comment #170)
> Created an attachment (id=252688) [details]
> Patch v13
> 
> Updated for textrun landing and other trunk changes.
> 
> I built and tested this on Linux and Mac. It seems to work very well, at least
> in non-scaling mode, for browsing regular sites. Print preview works fine in
> Linux. Mac printing may be slightly busted, but that's in flux right now
> anyway.
> 
> I think we can, and should, land this very soon; as soon as we open after
> alpha2 (i.e., next week) at the latest. There will no doubt be some little
> cosmetic regressions but I think we'll be fine as far as alpha-quality builds
> go.

Okay, that's fine with me.  I haven't seen anything more than minor cosmetic regressions, so it shouldn't be an issue to land.

Is there anything else I should do at this point?
I think we should probably just leave it alone until we land unless a significant new regression is found. There will probably be one more merge required just before we land.

romaxa: My v13 works on gtk2, maybe you can use it?
Comment on attachment 252704 [details] [diff] [review]
Updated to trunk (After gfx landing)

We're not going to put a non-cairo version of this patch on the trunk, so this discussion is not appropriate for this bug.  I'll send you an email shortly.
Attachment #252704 - Attachment is obsolete: true
v12 (v13 is just a subset of v12?) still contains code like

  ScaleRoundOut(1.0f / mContext->AppUnitsPerDevPixel())

These will all cause 1px rounding errors (don't know how critical they are)
for integral multiples of AppUnitsPerDevPixel. I'd suggest to create an
InvScaleRoundOut() which divides by the supplied parameter instead of
multiplying, and use

  InvScaleRoundOut(mContext->AppUnitsPerDevPixel())

instead. Then the result will be exact (unless compiled with -ffast-math or
similar).

BTW, NSToIntRound() causes problems too, but that is unrelated to this patch
(filed bug 368280).


+    mAppUnitsPerDevPixel = AppUnitsPerCSSPixel() / PR_MAX(1, dpi / 96);

Should be PR_MAX(1, (dpi + 48) / 96), see comment 121.

Sorry, v13 was an incomplete diff. I have a bad habit of doing that. I'll update to trunk and post a new patch.

Most of the time where we use ScaleRoundOut, we use it because it's explicitly OK to make the rectangle bigger than necessary. Of course, we probably do sometimes get it wrong, and making it bigger than necessary can cost performance too. One of the good things about this patch is that it gives us a more solid framework for fixing various kinds of rounding bugs.

Thanks for the reminder about the mAppUnitsPerDevPixel calculation. We probably should change it. It would be nice to make contact with people who have a range of exotic screens and get some feedback about exactly when pixel-doubling should kick in...
(In reply to comment #176)
> It would be nice to make contact with people who have a range
> of exotic screens and get some feedback about exactly when pixel-doubling
> should kick in...

When 150 dpi screens become usual, people probably want a pref for it (for the
48 in the formula, which should be the default). Would also be useful for
testing.

Will this bug fix resolution independent UI in OS X?  If not I will submit a new bug.

Currently it is broken.
I would think this would allow res independence, or at least be a blocker bug for it. As there are more and more devices appearing that will support different resolution independence this would be nice. I think the Nokia 770/n800 run at around 225 dpi, and the iPhone will be 160dpi I think plus MacOSX supporting it (probably for the iPhone) this would be nice.
Also there are some problems with changing DPI for SVG content, it is not changed at all.

Stil one problem with changing DPI for the text.
I tried to make small fix for this, it looks like:

****************************************************
mozilla/gfx/src/thebes/nsThebesFontMetrics.h
-  PRInt32 mAppUnitsPerDevUnit;

mozilla/gfx/public/nsIFontMetrics.h
+  PRInt32 mAppUnitsPerDevUnit;

mozilla/gfx/src/nsDeviceContext.cpp
-    if (fm->Font().Equals(aFont)) {
+    if (fm->Font().Equals(aFont) &&
+        mContext->AppUnitsPerDevPixel() == fm->mAppUnitsPerDevUnit) {

****************************************************

For setup any DPI value:
mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:
-    mAppUnitsPerDevPixel = AppUnitsPerCSSPixel() / PR_MAX(1, dpi / 96);
+    mAppUnitsPerDevPixel = ((float)AppUnitsPerCSSPixel() * 96.) / (float)dpi;



(In reply to comment #180)
> Also there are some problems with changing DPI for SVG content, it is not
> changed at all.

Yeah; I spent some time on that, but I never came up with a fix that seemed correct.  I'm not really sure where the SVG code decides what scaling factor to use for rendering when there isn't a width/height set.
 
> Stil one problem with changing DPI for the text.
> I tried to make small fix for this, it looks like:
> 
> ****************************************************
> mozilla/gfx/src/thebes/nsThebesFontMetrics.h
> -  PRInt32 mAppUnitsPerDevUnit;
> 
> mozilla/gfx/public/nsIFontMetrics.h
> +  PRInt32 mAppUnitsPerDevUnit;
> 
> mozilla/gfx/src/nsDeviceContext.cpp
> -    if (fm->Font().Equals(aFont)) {
> +    if (fm->Font().Equals(aFont) &&
> +        mContext->AppUnitsPerDevPixel() == fm->mAppUnitsPerDevUnit) {

Makes sense; it works? (I'm assuming this is in addition to a forced reflow.)  On a side note, it might be a good idea to make the DPI pref change listener force a reflow; I might add it eventually, although in general listening to the pref doesn't really make sense without some way to detect DPI changes.
> +        mContext->AppUnitsPerDevPixel() == fm->mAppUnitsPerDevUnit) {

With this changes we are not required to do force reflow... we just need to redraw layout (widget_redraw)....

Tested ;) at least for text elements it works fine.

The tree just closed for the 1.9 alpha 2 release, so we should land this as soon as we can after the tree opens again. Here's a plan: I'll update my tree to trunk one more time, verify that Linux, Windows and Mac all work OK, post the patch, give it some sort of review, and then let Eli update the patch as necessary and land it.
(In reply to comment #183)
> The tree just closed for the 1.9 alpha 2 release, so we should land this as
> soon as we can after the tree opens again. Here's a plan: I'll update my tree
> to trunk one more time, verify that Linux, Windows and Mac all work OK, post
> the patch, give it some sort of review, and then let Eli update the patch as
> necessary and land it.

Okay; I actually have a version of the patch synced to tip, just so you know.  Do you want me to post it?
Yes please. I'll compare it to mine. Also, I assume you've done some testing to make sure it's still working as expected on Windows? (I've just tested my patch on Mac and things seem OK, Linux next.)
Ok, there are some problems with changing DPI on fly ;)

data:text/html,<input type=submit value="blabla">
works ok after changing layout.css.dpi preference + window_redraw....

But if I will do reload for the page... button looks like "bold".. and not scaled properly... 

this not happens for next url:
data:text/html,<input type=submit value="blabla" style="width: 70; height: 30; font-size:16;">

Something done for Images in mozilla/content.... but nothing for other form controls... 

I'm trying to do changing of DPI on fly without any reflow... ;), in this case we can create same zoom (full page, and such fast), like in Opera and IE7.


Changing DPI on the fly is not the best way to do zooming, and I don't think it's a high priority for Firefox, although I guess we'd accept patches for it if it's not too much code.
But anyway it works wrong, if we will change DPI for  google.fi and do reload of the page... .   images and other elements change sizes, but there are some problems for input field and buttons... 
same not happens for ya.ru page, even after reloading the page.

(In reply to comment #188)
> But anyway it works wrong, if we will change DPI for  google.fi and do reload
> of the page... .   images and other elements change sizes, but there are some
> problems for input field and buttons... 
> same not happens for ya.ru page, even after reloading the page.

I have no idea how any layout data could survive a reload; is that really what's happening on ya.ru?

Native theme widgets are intentionally sized in device pixels.  That shouldn't apply to inputs or buttons, though.

(In reply to comment #187)
> Changing DPI on the fly is not the best way to do zooming, and I don't think
> it's a high priority for Firefox, although I guess we'd accept patches for it
> if it's not too much code.

Yeah, it's not really very useful for Firefox.  We could make a really cool slider thing that makes the UI change size, but it would be pretty worthless.  That said, I don't think it'll be very much code, so we could probably take it on the trunk.  (I think it should just be a restyle reflow plus clearing the font cache, and maybe a couple of other things I don't know about.)

Actually, Oleg, could you file a separate bug and mark it depending on this one?  It would make things much clearer.

(In reply to comment #185)
> Yes please. I'll compare it to mine. Also, I assume you've done some testing to
> make sure it's still working as expected on Windows? (I've just tested my patch
> on Mac and things seem OK, Linux next.)

Yeah, it's still working; I'll post it in a moment.
If you guys are ready to land this, we can keep the tree closed through tomorrow to give you a chance to do so -- just let me know.  Assuming the builds from today are ok we'll check in the version bump to 1.9a3 and then you guys should be good to land tomorrow morning (pacific), possibly even tonight.  I'd like to reopen the tree by tomorrow later afternoon pacific time though.
Attached patch Patch v14 (obsolete) — — Splinter Review 

    
    

    
  
A quick review of the changes in mozilla/layout/style/ in attachment 
252251 [review]:

The nsCSSGroupRule.cpp change seems unrelated to this patch.  Though the
only reasons not to include it are that the code in question should be
cleaned up further, and that it might make the history confusing.

Fixing the indentation in nsComputedDOMStyle::GetTextIndent, 
GetMaxHeight (twice), GetMaxWidth (twice), GetMinHeight, and
GetMinWidth, would be nice, though.

I noticed the change of rounding in
nsROCSSPrimitiveValue::SetAppUnits(float), although I suspect it's fine.

Seems like it would be good to file a followup bug on making nsCSSValue
not use twips anymore.

I also suspect you know what you're doing with changing nsStyleBorder
and nsStyleOutline from using the pres context's conversion to the
device context's, although I don't follow it.

    
    

    
  
(In reply to comment #192)
> A quick review of the changes in mozilla/layout/style/ in attachment 
> 252251 [details]:
> 
> The nsCSSGroupRule.cpp change seems unrelated to this patch.  Though the
> only reasons not to include it are that the code in question should be
> cleaned up further, and that it might make the history confusing.

I'll try to remember not to check that in.

> Fixing the indentation in nsComputedDOMStyle::GetTextIndent, 
> GetMaxHeight (twice), GetMaxWidth (twice), GetMinHeight, and
> GetMinWidth, would be nice, though.

Okay.

> I noticed the change of rounding in
> nsROCSSPrimitiveValue::SetAppUnits(float), although I suspect it's fine.

I'm pretty sure that rounding down percentage values is wrong.  Being off by a twip doesn't usually make a difference, though.

> Seems like it would be good to file a followup bug on making nsCSSValue
> not use twips anymore.

Okay; do you prefer inches?  Points?  Passing in a prescontext? Or something else?  (They're real twips because they come from physical units.)

> I also suspect you know what you're doing with changing nsStyleBorder
> and nsStyleOutline from using the pres context's conversion to the
> device context's, although I don't follow it.

Erm, I think you read that part of the patch backwards?  It doesn't look the least bit unusual to me.

    
    

    
  
I ran reftests and got two unexpected failures, one related to my test setup and one which was already a known problem on Mac, so I think that's OK.

Review comments:

nsEventListenerManager::PrepareToUseCaret and GetCoordinatesFor need to be documented that they return device pixel coordinates.

Somewhere we need to document that gfxContext is normally in (transformed) device coordinates, so the caller of gfxContext methods is responsible for applying an appunit to device-pixel conversion. I think this is ugly and counterintuitive --- if you transform to device coordinates you don't expect scaling, translation or rotation happen after that --- but that's the way it is for now.

I'm a bit concerned about nsCanvasRenderingContext2D::Render. It uses mWidth/mHeight as if they were device pixels, but they're actually CSS pixels. We need a followup bug to fix <canvas> for high-DPI situations. (Basically we need to make the internal surface width/height match the target device's device pixels, so that scaling happens when script draws into the canvas, not at render time.)

Index: layout/generic/nsHTMLCanvasFrame.cpp
   nsSize canvasSize = GetCanvasSize();
+  nsSize sizeAppUnits(GetPresContext()->DevPixelsToAppUnits(canvasSize.width),
+                      GetPresContext()->DevPixelsToAppUnits(canvasSize.height));

You've made GetCanvasSize return CSS pixels (which should be documented somewhere), so these should be CSSPixelsToAppUnits. Also, you're not actually using sizeAppUnits anywhere --- still using canvasSize --- which causes canvas to not work at all. I fixed my patch to use sizeAppUnits instead and canvas works again.

+  * Get/set the print scaling level

Could you say something in the comment about when it's safe to call SetPageScale and what it does?

Index: layout/generic/nsHTMLReflowState.cpp
+    mComputedPadding.top = nsPresContext::CSSPixelsToAppUnits(mComputedPadding.top);
+    mComputedPadding.right = nsPresContext::CSSPixelsToAppUnits(mComputedPadding.right);
+    mComputedPadding.bottom = nsPresContext::CSSPixelsToAppUnits(mComputedPadding.bottom);
+    mComputedPadding.left = nsPresContext::CSSPixelsToAppUnits(mComputedPadding.left);

+      nsPresContext::CSSPixelsToAppUnits(mComputedBorderPadding.top);
     mComputedBorderPadding.right =
-      NSIntPixelsToTwips(mComputedBorderPadding.right, p2t);
+      nsPresContext::CSSPixelsToAppUnits(mComputedBorderPadding.right);
     mComputedBorderPadding.bottom =
-      NSIntPixelsToTwips(mComputedBorderPadding.bottom, p2t);
+      nsPresContext::CSSPixelsToAppUnits(mComputedBorderPadding.bottom);
     mComputedBorderPadding.left =
-      NSIntPixelsToTwips(mComputedBorderPadding.left, p2t);
+      nsPresContext::CSSPixelsToAppUnits(mComputedBorderPadding.left);

These should all be devpixels, right?

Ditto in layout/xul/base/src/nsBox.cpp, nsBox::GetPadding/nsBox::GetBorder.

           if (aEvent->message==NS_TEXT_TEXT) {
-            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.x=NSTwipsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.x, t2p);
-            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.y=NSTwipsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.y, t2p);
-            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.width=NSTwipsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.width, t2p);
-            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.height=NSTwipsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.height, t2p);
+            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.x=NSAppUnitsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.x, p2a);
+            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.y=NSAppUnitsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.y, p2a);
+            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.width=NSAppUnitsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.width, p2a);
+            ((nsTextEvent*)aEvent)->theReply.mCursorPosition.height=NSAppUnitsToIntPixels(((nsTextEvent*)aEvent)->theReply.mCursorPosition.height, p2a);
           }
           if((aEvent->message==NS_COMPOSITION_START) ||
              (aEvent->message==NS_COMPOSITION_QUERY)) {
-            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.x=NSTwipsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.x,t2p);
-            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.y=NSTwipsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.y,t2p);
-            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.width=NSTwipsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.width,t2p);
-            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.height=NSTwipsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.height,t2p);
+            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.x=NSAppUnitsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.x, p2a);
+            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.y=NSAppUnitsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.y, p2a);
+            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.width=NSAppUnitsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.width, p2a);
+            ((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.height=NSAppUnitsToIntPixels(((nsCompositionEvent*)aEvent)->theReply.mCursorPosition.height, p2a);
           }
           if(aEvent->message==NS_QUERYCARETRECT) {
-            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.x=NSTwipsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.x,t2p);
-            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.y=NSTwipsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.y,t2p);
-            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.width=NSTwipsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.width,t2p);
-            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.height=NSTwipsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.height,t2p);
+            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.x=NSAppUnitsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.x, p2a);
+            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.y=NSAppUnitsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.y, p2a);
+            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.width=NSAppUnitsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.width, p2a);
+            ((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.height=NSAppUnitsToIntPixels(((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect.height, p2a);
           }

I think these should all be converting to devpixels.

You need to rev the UUID in nsIPrintOptions.

The nsStyleBorder and nsStyleOutline changes are just to ensure that the rounding of border and outline widths that occur in the style system are rounding to device pixels, not CSS pixels, which makes sense to me.

Fix those and I think we're good to land. I just need to compare my patch against yours to see if I slipped in any fixes that you need.

    
    

    
  
+  nscoord newX = mHandScrollStartScrollX + NSIntPixelsToAppUnts(deltaX, p2a);
+  nscoord newY = mHandScrollStartScrollY + NSIntPixelsToAppUnts(deltaY, p2a);

typo "Unts"

In nsThebesContext::SetDPI, OSVal is unused on Mac. You might want to just declare it where it's assigned for GTK2 and Windows.

That's all I have. Good to go!

    
    

    
  
Eli, what times would you be available to land this in the window vlad gave in comment 190?

(And, FWIW, the structure that survives reload in comment 189 is probably the device context.  I've been seeing problems with dynamic DPI changes (which result from screen size changes when I dock/undock) ever since the switch to cairo.  But that belongs in other bugs.)

    
    

    
  
BTW, a followup idea for this: We should expose the current CSS-to-device-pixel ratio in the useragent string we sent to HTTP servers. This would allow them to send an image of the appropriate resolution.

    
    

    
  
(In reply to comment #194)
> Index: layout/generic/nsHTMLCanvasFrame.cpp
>    nsSize canvasSize = GetCanvasSize();
> +  nsSize sizeAppUnits(GetPresContext()->DevPixelsToAppUnits(canvasSize.width),
> +                     
> GetPresContext()->DevPixelsToAppUnits(canvasSize.height));
> 
> You've made GetCanvasSize return CSS pixels (which should be documented
> somewhere), so these should be CSSPixelsToAppUnits. Also, you're not actually
> using sizeAppUnits anywhere --- still using canvasSize --- which causes canvas
> to not work at all. I fixed my patch to use sizeAppUnits instead and canvas
> works again.

Yes, I messed up by not using sizeAppUnits everywhere.  However, it's definitely supposed to be DevPixelsToAppUnits.  Otherwise, the canvas doesn't get scaled up properly.

    
    

    
  
This seems to have broken balsa (gcc 3.4) and SunOS' nba & putt builds (SeaMonkey). And also TB win tbox & linux crazyhorse builds.

    
    

    
  
(In reply to comment #199)
> This seems to have broken balsa (gcc 3.4) and SunOS' nba & putt builds
> (SeaMonkey). And also TB win tbox & linux crazyhorse builds.

See bug 369588.

    
    

    
  
I think that this bug is a cause of bug 369618.

    
  
No longer blocks: 353860
Depends on: 353860

    
    

    
  
(In reply to comment #132)
> One issue I'm seeing with the patch is that there is a border missing for the
> inactive tabs in Firefox, see:
> http://wargers.org/mozilla/bug177805_units/177805_units_tabbar.htm

All tabs are shifted down. This works as a workaround:

.tabbrowser-tab {
  margin-bottom: 1px !important;
}
Depends on: 369684
Depends on: 369690
Depends on: 369692
Depends on: 369693
No longer depends on: 369693
No longer depends on: 369692
Depends on: 369698

    
    

    
  
After this patch is landed, the gtk2 toolkit doesn't build any more on Solaris 10. Is there any plan to fix this problem?

    
    

    
  
(In reply to comment #203)
> After this patch is landed, the gtk2 toolkit doesn't build any more on Solaris
> 10. Is there any plan to fix this problem?

With this patch, we only support cairo-gtk2.  That is completely intentional.  (We've been planning to break non-Thebes GFX for a while now; this patch is the first to break it.)   If there's something that breaking specifically on Solaris, file a bug; however, that seems unlikely.

    
    

    
  
So the point is that the configuration of those tinderboxes needs to be fixed to a supported one.  see also bug 369588 for a similar issue.

    
    

    
  


  

        Attachment #252688 -
        Attachment is obsolete: true

        Attachment #254214 -
        Attachment is obsolete: true

    
    

    
  
And marking fixed; yay :)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Depends on: 369864

    
  
Depends on: 370006

    
  
Blocks: 370014

    
  
Depends on: 369882
Depends on: 370444
Depends on: 370553
Depends on: 370629

    
  
Depends on: 370466

    
  
Depends on: 371551

    
  
No longer depends on: 371551

    
    

    
  
Could this have caused bug 370631?
Depends on: 370631
Depends on: 373381
Target Milestone: --- → mozilla1.9alpha3

    
  
Depends on: 376690

    
  
No longer depends on: 369864
Depends on: 380438
Depends on: 381074

    
    

    
  
I conclude from the testing I did with the nightlies that this bug caused the regression in bug 382458

Depends on: 382458

    
  
Blocks: 386406

    
  
No longer blocks: 386406
Depends on: 386406
Depends on: 391868
Depends on: 395677
Depends on: 396811
Depends on: 401904

    
  
Depends on: 398336

    
  
Depends on: 412646
Depends on: 421700
Depends on: 428975
Depends on: 434841

    
  
Blocks: 500997

    
  
No longer blocks: 500997
Depends on: 500997

    
  
Depends on: 513837

    
    

    
  
The way you are rounding floats to integers is not correct and still produces strange cumulative effects (even if there's no more any 1-device-pixel roundoff error).

Apparently you have fixed it using everywhere the function
round(x) = ceil(x + 0.5)

But in my opinion, the standard IEEE rounding is far better (and it is available in hardware): it rounds to the nearest EVEN integer when the diretion of rounding is not clearly decidable (equal absolute difference between the rounded result and the unrounded parameter).

This becomes far more important now that SVG starts being integrated in HTML5, and zooming to arbitrary scales though a transform funtion is part of its design: the effect of rounding will be visible as well when zooming scaled bitmap images (see the discussions about this within graphics 3D engines and how mipmaps are used):

ONLY the standard IEEE round-to-nearest-even mode (which is also normally the default rounding mode for floatting-point computings in strict mode) produces the correct isotropic rounding.

In other words, don't use the ceil() function (which is also costly in terms of hardware acceleration in CPUs or GPUs), just use the IEEE rounding mode.

Or at least to round on the nearest even half-pixel still using this IEEE rounding mode to then round this number of half-pixels to an integer number pixels, if you still want some compatibility with the current solution. In other words use:

  round(x) = IEEEroundtoeven(IEEEroundtoeven(x * 2.0) / 2.0)

(Note that the IEEE rounding mode produces very desirable isotropic effects when handling negative coordinates, which will occur because of zooming and various transforms, as it gives a perfect symetry around 0, producing perfect symetries around any other arbitrary center, when objects are later translated. Note also that works are already being done to integrate 3D objects and projections to 2D within SVG, using 4D transform matrixes, exactly like what is done within GPUs.)

Examples:

  x        |  x * 2.0  | IEEE round to | final IEEE
           |           | nearest even  | round(/2.0)
 ----------+-----------+---------------+------------
  0.00...0 |  0.00...0 |  0.0          |  0.0
 -0.00...0 | -0.00...0 | -0.0          | -0.0
           |           |               |
  0.00...1 |  0.00...1 |  0.0          |  0.0
 -0.00...1 | -0.00...1 | -0.0          | -0.0
 ----------+-----------+               |
  0.24...9 |  0.49...9 |  0.0          |  0.0
 -0.24...9 |  0.49...9 | -0.0          | -0.0
           |           |               |
  0.25...0 |  0.50...0 |  0.0          |  0.0
 -0.25...0 | -0.50...0 | -0.0          | -0.0
           |           +---------------+
  0.25...1 |  0.50...1 |  1.0          |  0.0
 -0.25...1 | -0.50...1 | -1.0          | -0.0
 ----------+-----------+               |
  0.49...9 |  0.99...9 |  1.0          |  0.0
 -0.49...9 | -0.99...9 | -1.0          | -0.0
           |           |               |
  0.50...0 |  1.00...0 |  1.0          |  0.0
 -0.50...0 | -1.00...0 | -1.0          | -0.0
           |           |               |
  0.50...1 |  1.00...1 |  1.0          |  0.0
 -0.50...1 | -1.00...1 | -1.0          | -0.0
 ----------+-----------+               |
  0.74...9 |  1.49...9 |  1.0          |  0.0
 -0.74...9 | -1.49...9 | -1.0          | -0.0
           |           +---------------+-----------
  0.75...0 |  1.50...0 |  2.0          |  1.0
 -0.75...0 | -1.50...0 | -2.0          | -1.0
           |           |               |
  0.75...1 |  1.50...1 |  2.0          |  1.0
 -0.75...1 | -1.50...1 | -2.0          | -1.0
 ----------+-----------+               |
  0.99...9 |  1.99...9 |  2.0          |  1.0
 -0.99...9 | -1.99...9 | -2.0          | -1.0
           |           |               |
  1.00...0 |  2.00...0 |  2.0          |  1.0
 -1.00...0 | -2.00...0 | -2.0          | -1.0
           |           |               |
  1.00...1 |  2.00...1 |  2.0          |  1.0
 -1.00...1 | -2.00...1 | -2.0          | -1.0
 ----------+-----------+               |
  1.24...9 |  2.49...9 |  2.0          |  1.0
 -1.24...9 | -2.49...9 | -2.0          | -1.0
           |           |               |
  1.25...0 |  2.50...0 |  2.0          |  1.0
 -1.25...0 | -2.50...0 | -2.0          | -1.0
           |           +---------------+
  1.25...1 |  2.50...1 |  3.0          |  1.0
 -1.25...1 | -2.50...1 | -3.0          | -1.0
 ----------+-----------+               |
  1.49...9 |  2.99...9 |  3.0          |  1.0
 -1.49...9 | -2.99...9 | -3.0          | -1.0
           |           |               |
  1.50...0 |  3.00...0 |  3.0          |  1.0
 -1.50...0 | -3.00...0 | -3.0          | -1.0
           |           |               |
  1.50...1 |  3.00...1 |  3.0          |  1.0
 -1.50...1 | -3.00...1 | -3.0          | -1.0
 ----------+-----------+               |
  1.74...9 |  3.49...9 |  3.0          |  1.0
 -1.74...9 | -3.49...9 | -3.0          | -1.0
           |           +---------------+-----------
  1.75...0 |  3.50...0 |  4.0          |  2.0
 -1.75...0 | -3.50...0 | -4.0          | -2.0
           |           |               |
  1.75...1 |  3.50...1 |  4.0          |  2.0
 -1.75...1 | -3.50...1 | -4.0          | -2.0
 ----------+-----------+               |
  1.99...9 |  3.99...9 |  4.0          |  2.0
 -1.99...9 | -3.99...9 | -4.0          | -2.0
           |           |               |
  2.00...0 |  4.00...0 |  4.0          |  2.0
 -2.00...0 | -4.00...0 | -4.0          | -2.0
           |           |               |
  2.00...1 |  4.00...1 |  4.0          |  2.0
 -2.00...1 | -4.00...1 | -4.0          | -2.0
 ----------+-----------+               |
  2.24...9 |  4.49...9 |  4.0          |  2.0
 -2.24...9 | -4.49...9 | -4.0          | -2.0
           |           |               |
  2.25...0 |  4.50...0 |  4.0          |  2.0
 -2.25...0 | -4.50...0 | -4.0          | -2.0
           |           +---------------+
  2.25...1 |  4.50...1 |  5.0          |  2.0
 -2.25...1 | -4.50...1 | -5.0          | -2.0
 ----------+-----------+               |
  2.49...9 |  4.99...9 |  5.0          |  2.0
 -2.49...9 | -4.99...9 | -5.0          | -2.0
           |           |               |
  2.50...0 |  5.00...0 |  5.0          |  2.0
 -2.50...0 | -5.00...0 | -5.0          | -2.0
           |           |               |
  2.50...1 |  5.00...1 |  5.0          |  2.0
 -2.50...1 | -5.00...1 | -5.0          | -2.0
 ----------+-----------+               |
  2.74...9 |  5.49...9 |  5.0          |  2.0
 -2.74...9 | -5.49...9 | -5.0          | -2.0
           |           +---------------+-----------
  2.75...0 |  5.50...0 |  6.0          |  3.0
 -2.75...0 | -5.50...0 | -6.0          | -3.0
           |           |               |
  2.75...1 |  5.50...1 |  6.0          |  3.0
 -2.75...1 | -5.50...1 | -6.0          | -3.0
 ----------+-----------+               |
  2.99...9 |  5.99...9 |  6.0          |  3.0
 -2.99...9 | -5.99...9 | -6.0          | -3.0
           |           |               |
  3.00...0 |  6.00...0 |  6.0          |  3.0
 -3.00...0 | -6.00...0 | -6.0          | -3.0

Note that even in this case, the second IEEE rounding to nearest even is stable: it returns exactly the same integers as if a single IEEE rounding to nearest even was used, without first rounding to integer numbers of half-pixels.

In other words, a single rounding function can be used to produce fully isotropic images, independantly of their visual translation:

  round(x) = IEEEroundtoeven(x)

And thanks to CPU- and GPU-builders, it is already hardware-accelerated; so builtin intrinsic C/C++ functions that map to these hardware instructions, so this should perform really fast ! (as long as the CPU or GPU is instructed to use this rounding mode).

    
    

    
  
Also note that the ceil() and floor() functions do not benefit as much of the hardware acceleration (in addition to being anisotropic, a undesirable effect for producing images).

This is because (conceptually) the ceil or floor functions are defined as:

 floor(x) = (x < 0) ? -positiveCeil(-x) : positiveFloor(x);
 ceil(x) = (x < 0) ? -positiveFloor(-x) : positiveCell(x);

And hardware does not like performing test signs, so it is often not implemented (on the opposite, the IEEE round-to-nearest-even mode only requires testing a single bit in the mantissa, depending on the integer value of the base-2 exponent, independantly of the sign of the number to round).

There will then be no intrinsic builtin for these functions, and the C/C++ compiled code will generate conditional-branches, which defeats the branch prediction and slows down the performance.

Note that this IEEE rounding mode is the STANDARD DEFAULT mode for ANSI/ISO C/C++ when computing with all floatting-point types (float, double, long double, as well as the newer fixed decimal types) and there are excellent reasons for this rounding mode being the default (notably for numbers with fixed precision, which may be implemented using integer arithmetics with the ALU only and not the FPU, or using parallel vector extensions heavily used in 3D kernel renderers).

    
    

    
  
Also, don't use the division by 2.0 (as described above) but a multiplication (this will help C/C++ optimizers if they don't detect it).

  inline long double _Gfx_Fastest_IsotropicRoundLD(long double x) {
    return IEEEroundtoeven(x); // look for actual builtin intrinsic if available
  }
  inline long double _Gfx_FastCompatible_IsotropicRoundLD(long double x) {
    return _Gfx_Fastest_IsotropicRoundLD(
      _Gfx_Fastest_IsotropicRoundLD(x * 2.0) * 0.5);
  }

  inline double _Gfx_Fastest_IsotropicRoundD(double x) {
    return (double)_Gfx_Fastest_IsotropicRoundLD((long double)x);
  }
  inline double Gfx_FastCompatible_IsotropicRoundD(double x) {
    return (double)_Gfx_FastCompatible_IsotropicRoundLD((long double)x);
  }

  inline float _Gfx_Fastest_IsotropicRoundF(float x) {
    return (float)_Gfx_Fastest_IsotropicRoundIEEEroundtoevenLD((long double)x);
  }
  inline float _Gfx_FastCompatible_IsotropicRoundF(float x) {
    return (float)_Gfx_FastCompatible_IsotropicRoundLD((long double)x);
  }

Check the internal macros or builtin functions that define the IEEEroundtoeven() function as it is just conceptual here.

    
    

    
  
> as it gives a perfect symetry around 0

This is a drawback, not a benefit.  The key to using floor(x + 0.5) is that this gives consistent rounding direction and avoids "missing pixel" seams.  If we were to actually use round-to-nearest-ties-to-even, then markup like this:

  <div>
    <div style="height: 0.5px"></div>
    <div style="height: 0.5px"></div>
    <div style="height: 0.5px"></div>
    <div style="height: 0.5px"></div>
  </div>

would result in the four inner divs spanning the vertical ranges [0, 0), [0,1), [1,2), [2, 2).  That is, things specified to be the same size in the source would NOT be the same size in the rendering.

You may want to also read http://weblogs.mozillazine.org/roc/archives/2008/02/rounding_toward.html

    
    

    
  
If you have only 2 device pixels, you can only create 2 vertical spannings (you can't have 4).

The IEEE rounding mode will still result in exactly 2 non overlapping pixels that cover the full range correctly.

It's simply IMPOSSIBLE to have 4 equal vertical ranges on the device.

And perfect symetry around 0 is highly desirable in almost all cases (including the fact that symetry will behave correctly for object borders that should be as precisely as possible position on the pixel boundaries, without loosing these borders).

If you have subpixel precision at the device level, treat them with the precision of these subpixels, but you should still still use the IEEE round-to-nearest-even-tie mode (but subpixel precision does not even exist for now on the vertical axis on display, as subpixel precision is typically used only for flat LCD/LED panels; but it exists on printing devices like bubble jet printers)

With floor (x + 0.5), not only this is costly in terms of CPU/FPU or GPU instructions, but you get undesirable border effects when computing coordinates in complex layouts with floatting point values.

And what you finally get ONLY when source coordinates where EXACTLY computed as (0.0, 0.5, 1.0, 1.5) will be the integer ranges:
  [0, 1), [1, 1), [1, 2), [2, 2)
So you'll still have only 2 non-zero-width ranges (the 1st and the 3rd). So here also the same sizes in the source will result in distinct sizes in the rendering. Your argument is not valid.

But any very tiny difference in the source floatting-point coordinates will cause the 2 visible ranges to be selected randomly within these 4 ranges (depending on the precision of the coordinates), producing anisotropic images.

That's something that the IEEE rounding mode to nearest even tie completely avoids in almost all cases. This IEEE rounding mode is then much more consistant, as it will correctly round all the lowest differences that exist when the computed floatting-point coordinates have only a few ups of differences that are mich smaller than the device precision.

    
    

    
  
> If you have only 2 device pixels

What made you think that's the case?  There are various things that need to be rounded to _CSS_ pixels, which can be multiple device pixels in size.

> And perfect symetry around 0 is highly desirable in almost all cases

Except the case when there is a fundamental anisotropy to the content.  Like say a web page (which has a clearly defined up and down for the most part; SVG can be an exception).

Please do read roc's blog post.  He explains what the issues are much more clearly than my simple (and somewhat flawed) example does.

    
    

    
  
Note that your link to the mozilla article just discusses the case of rounding towards or away from zero. It DOES NOT disacuss about the rounding modes towards even ties, which is definitely NOT harmful

And you should read articles about 3D rendering, about why it is much better to use it rather than floor(x+0.5) towards minus infinity of equal ties, which produces also other effects like incorrect colors when zooming out thin black and white patterns at 50%: you will get a full-white or full-black fill, and this will constantly flicker between those two when slowly translating such patterns with subpixel floatting-point positions

With the IEEE rounding mode set to nearest even tie, you'll get a consistant pattern of black and white at ALL positions, when not using smoothed rendering, or you'll be able to compute consistant gray levels with a smoothed rendering, except within a few ups at a precision MUCH smaller than the device pixel precision, i.e. a level of precision where the difference of smoothing greys will be almost invisible.]

The typical case occurs when computing floatting points coordinaes like:

 double w3 = 3/7;
 double w1 = 1/7;
 double w2 = 2/7;
 return w3 - w1 - w2;

which may return a small negative number (such as -1e-12), due to limited precisions : the difference will come from the few ups of precision used to represent the three independantly stored values w1, w2, w3.

If you round this result with floor(x+0.5), you'll see that it rounds to -1 (instead of the expected exact 0 that you'll get with IEEE rounding mode towards even tie) and things that should be completely invisible when rendered will suddenly become visible.

    
    

    
  
And in the blog, you'll see the important comment made by Dijskra.

He is right when saying that the IEEE round to nearest-even-tie is FAR better than floor(x+0.5) or ceil(x+0.5) suggested in the blog article to replace the rounding modes toward zero used in the C/C++ basic typecasts of float/double to integer types.

Reconsider your position, and create another article stating that floor(x+0.5) and ceil(x+0.5) is ALSO considered harmful for graphics rendering.

    
    

    
  
Note also that the IEEE 754 round-to-nearest even tie is also mandatory in font renderers (notably those performing correct hinting), this is not just for SVG, and the SVG integration within HTML will continue to grow in HTML5.

There's also no such concept as "CSS pixels" which are also arbitrary floatting point measurement units that have nothing in common with the device precision, where rescaling will occur anyway using all sorts of scaling factors that are definitely not part of the CSS specifications (not even in CSS 3).

    
    

    
  
> And in the blog, you'll see the important comment made by Dijskra.

Are you sure you're not misattributing Peter Moulder's comment?

> for graphics rendering

Where does graphics rendering come in?  The discussion in this bug, for the most part, and in the blog post, is about _text_ and border rendering and rendering of websites, not graphics rendering.  Graphics rendering is a different ballgame, quite possibly.  Certainly different constraints apply.

    
    

    
  
> There's also no such concept as "CSS pixels"

There is, in fact.  CSS defines a unit called "pixel" which is only very loosely related to device pixels.  However some things in practice (border widths come to mind) have to be rounded to integer numbers of CSS pixels.

    
    

    
  
Rounding towards even is the same as rounding towards zero for half the midpoint values and rounding away from zero for the other half.  So it has exactly the issues roc's blog post describes.

> The typical case occurs when computing floatting points coordinaes like:

Note that we currently do not in fact use floating point coordinates internally

> which may return a small negative number (such as -1e-12),
...
> If you round this result with floor(x+0.5), you'll see that it rounds to -1

That statement is false.  It will round to 0.

    
    

    
  
And sorry for my example, I forgot to substract 1/4 to the computed expression, or to divide the result of the computed differences by 2 (this is effectively where the undesirable and unstable cutoffs occur).

You should also read the new specification published for ECMAScript, which very precisely describes the precision needed to compute Number values (which are directly tied to IEEE-754 64-bit doubles, and mandates the use of rounding towards nearest even tie, for minimizing the ups errors.

You should read the specification of JPEG 2000, PNG, MPEG, H.264, and Ogg Video formats that also have the same constraints for the prefered rounding mode (remember that HTML 5 will integrate these media types very tightly).

The concept of "CSS pixels" mapping to a fixed integer number of device pixels is simply flawed. "CSS pixels" are floatting point measures that JUST need to have AT LEAST a IEEE-754 64-bit precision.

A higher precision should be accepted, but ECMAScript still does not support Number values with higher precisions, even if ECMAScript interoperates through OMG IDL interfaces that define them precisely, and even if there's hardware support for them (for example "long double" 80-bit precision in C/C++ and on x86  FPUs, or high-end GPUs).

    
    

    
  
> Note that we currently do not in fact use floating point coordinates internally

But pages are designed to use them, as they are exposed through DOM and ECMAScript that already use flotting points. That's where the precision ups will occur when computing coordinates, and this is the only world where "CSS pixels" are measured, independantly of the target rendering device.

The DOM, or scripts within pages, should NEVER have to be exposed with values modified and remapped to integers by the renderer, they should just be created using the standard double precision as specified in ECMAScript, and they will produce consistant results on all browsers or renderers or target surface. In other words, they should retain their full double precision.

    
    

    
  
(In reply to comment #223)
> You should also read the new specification published for ECMAScript, which very
> precisely describes the precision needed to compute Number values (which are
> directly tied to IEEE-754 64-bit doubles, and mandates the use of rounding
> towards nearest even tie, for minimizing the ups errors.

I assure you, SpiderMonkey's number representation system (and calculation methods and their implementation) does not overlap with Gecko's layout-dimension representation in any way.  :-)

    
    

    
  
> this is the only world where "CSS pixels" are measured, independantly of the
> target rendering device.

Why are we imposing this arbitrary constraint?  The target rendering device is very important.

> should NEVER have to be exposed with values modified and remapped to integers
> by the renderer

Good luck.  Every single renderer does this, for various good reasons (mostly to do with the fact that scripts tend to try to impose self-contradictory constraints; I suggest you google around for the long discussions that happened about this).
 
> they should just be created using the standard double precision as specified
> in ECMAScript

We've considered using doubles for internal coordinates.  There are issues that need to be worked out with rounding artifacts (which round-to-even does NOT address) and there is a noticeable memory/performance cost last it was measured.

> and they will produce consistant results on all browsers

No, they won't.  It's much harder to get consistent results with floating point, in fact, because suddenly the order of arithmetic operations in the layout algorithms matters.

    
    

    
  
One other note.  The most common way in which coordinates enter the system is via stylesheets and CSS does not use double-precision floating-point.  It uses (due to limitations like finite memory) arbitrary rational coordinates represented via decimal expansions.  In practice, renderers round these to some sort of integer unit instead of actually implementing infinite-precision rational arithmetic; we do it to 1/60 of a CSS pixel, while others round to integer CSS pixels or whatnot.

    
    

    
  
Isn't CSS directly exposed via OMG IDL for interoperability with scripting languages ? CSS defines two levels:

- a syntaxic source language on the surface which uses finite decimal precision. That precision is effectively limited by source size. This syntaxic language however is not mandatory, this is just used for interchange of source code, rather than of binary objects.

- a IDL interoperability level which is its runtime object representation: the CSS language will be anyway always complied to this object form. Scripts do not have to serialize or generate the language level, given that this can be done dynamically by serialization (basicall the generic Object::toString() method).

CSS should interact cleanly with ECMAScript, without any losses or differences of precision, so it should allow correct rounding with IEEE-754 64-bit doubles, at least (but other scripting engines may support higher precisions), according to ECMAScript specs which define the precision of builtin mathematical operators and functions in the Math object with extreme care about the acceptable number of ups of error.

Webpages (and other medias like SVG, PNG, JPEG2000, MPEG...) will be designed according to these precision requirements (even if some media types also use other numeric representations with lower precision), and SVG and some media types are currently being fully integrated within HTML 5. I can't imagine that HTML 5 will not require at least the standard IEEE-754 64-bit precision as documented in ECMAScript.

Now you can do whatever you want in the renderer when mapping coordinates to device units, but this should ABSOLUTELY NOT affect what is perceived in scripts or through the IDL interface to the style member of HTMLElement objects.

So the renderer MUST NOT modify these IDL-reflected values, even if it convertes them internally to device-specific integers after performing some device-specific transforms, notably rescaling to the device resolution, and pixel-grid alignments (and this is only after this coordinates transform has been done that correct rounding to integers of device units should occur).

    
    

    
  
> Isn't CSS directly exposed via OMG IDL 

No.

> CSS defines two levels

No, CSS itself, as defined by the CSS working group, only defines the first of the two things you list.  A second group of people, who didn't actually understand CSS very well, then went and grafted an IDL API on top of it.  And since they were basically building on top of the existing CSS syntax, IDL access to CSS is on a string level only; everything crossing that boundary from JS is first serialized to a string and then that string is parsed following the syntactic language rules.  Now there are plans to create new CSS-related IDL APIs, but the above describes the state of things today.

I seriously suggest looking into the actual APIs the CSSOM exposes instead of just assuming what they look like.  I also suggest generally familiarizing yourself with things before making completely incorrect claims...

    
    

    
  
The second workgroup you're speaking about is also within the W3C.

The CSS API level is described in SAC, and it is a FULL part of the W3C Style Activity, with bindings being defined for Java, C (extensible to C++), and Perl.

http://www.w3.org/Style/CSS/SAC/

and yes CSSOM is now part of DOM Level 2 (it should be mandatory for HTML 5), whose SAC describes concrete interfaces for a few languages. CSSOM is alread at the stage of Candidate Recommandation.

You can't say that this was defined by a random separate group, given that the W3C is already adapting all its past specifications to define their interoperation in terms of OMG IDL

Well, in terms of IDL, SAC (v.1.3) provides effectively the LexicalUnit object that has several properties: a string representation of the value, an extractable unit (from a small enumerated list), and two numeric value extractors: as an integer, or a a 32-bit float (it's strange that they did not include a 64-bit double, similar to what is used in ECMASCript Number, but ECMAScript will still be able to interact with the string serialization of the LexicalUnit object).

My opinion is that a future version of SAC should extend this interface to use a more direct LexicalUnit::getDoubleValue() which would make it significantly faster for ECMAScript interoperability, and an even faster as well for processing in Java, C, C++, .Net, Perl and other engines, given that it already defines and supports a limited getFloat() value extractor and the lexicalUnitType() value extractor, in addition to the getStringValue() serialization plus two incrementing/decrementing operations (mostly for list counters).

And anyway, in CSSOM, the CSSPrimitiveValue object also exposes the enumerated unitType value property (readonly) and the same floatValue which can be set or retrieved, or the combined stringValue property. That is a damn'ed slow interface if this is just the one exposed, and in fact if this is the only supported representation, it has a possibly infinite decimal precision that scripts will never be able to compute.

Note also that there's not even any public constructor defined in the interface for LexicalUnit. The only way it can be instanciated is through the InputSource object constructors taking a string or characterStream in parameter, in order to compile it to a stylesheet containing lists of Selectors. It's really inefficient in practice, and actual implementations will necessarily add (and use) concrete constructors of objects implementing the LexicalUnit interface.

In CSSOM at least there's a concrete isolated LexicalUnit object but CSSOM is just one concrete implementation of SAC and browsers with ECMAScript bindings for DOM Level 2 will implement SAC using a class implementing the LexicalUnit interface with a concrete constructor taking a Number and a unit type, even if they operate with other engines through the basic (readonly) interfaced accessors. Oh... well... CSS should really define a standard minimum supported precision for the lexical units.

Anyway CSS 2.1 clearly states that computed valus are expected to be rounded when converted to device pixels, but it does not describe the operation, given that it does not fix their geometry (they are not necessarily in square grids or with a constant size, or arranged in horizontal rows and vertical columns. But if you implement this coordinates mapping, the deault IEEE 754 rounding mode should be used to avoid border effects.

    
    

    
  
SAC has nothing to do with the CSS working group, and I believe it's a dead project.  (I'm a member of the CSS working group.)

Also, this discussion doesn't belong on this bug, which was fixed a long time ago.

    
    

    
  
Someone moved my initial comment there, I actually posted in an open bug that was unified with this one (and the last post before mine refers to a bug redirected here.

And SAC, even if it looks "dead", is still what CSSOM implements concretely. And both are supported by the W3C, which has not removed either of them. It remains valid as a basic interface and usable even when interoperating with the ongoing CSS 3 proposals. You can easily derive a SAC 3.0 interface from the SAC 1.3 interface, to add additional services for CSS 3.0 (and I wish that it defines a doubleValue property, implemented in CSSOM 3.0, for use in CSS 2.1 or 3.0 as well, and directly usable with ECMAScript Number values without using the slow interface with strings only).

But even without this change, the dual exposition of the numeric value as a 32-bit int or as a 32-bit float means that it can represent at least all coordinates with 24-bit integers. This clearly limits the number of significant decimal digits supported in the string interface to 7 at most plus a sign.

For higher precisions, only the slow string interface is interoperable); so yes, "CSS pixels" in most documents can't be more precise than 0.01px, if documents cannot be wider or higher than 5 digits (less than 100000px) if using the limited float precision (such limited precision means that zooming in the document at moderate scale 50:1 or higher will very easily exhibit the roundoff errors).

And because of this insufficient precision of 32-bit floats, the effect of the rounding mode to choose when computing images or layouts is EVEN MORE CRITICALLY IMPORTANT !

    
    

    
  
And effectively this bug is not CLOSED given that its full resolution still depends on open bugs, and it is also blocking other open bugs.

In its state, it is just temporarily FIXED, within some limits that do not fully cover all the open bugs on which it depends.

    
    

    
  
verdy_p, I can only make several recommendations:

1)  I'm happy to discuss the politics and realities of W3C stuff (which you seem
    to be _very_ confused about) with you in the right forum (private mail is
    fine).  This bug is not the right forum.
2)  I'm not sure what makes you think you know better than the person who filed a
    bug whether that bug is fixed.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: