Closed Bug 925611 Opened 11 years ago Closed 10 years ago

size attributes cut-off image edges when the size is not the same as actual size of the image.

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Version:
20 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34
Tracking Flags:
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- wontfix

People

(Reporter: kjhgfnbmbjgiyut, Assigned: seth)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(11 files, 9 obsolete files)

An image used for all tests. This is a jpg-image with 1px blue border.
(deleted), image/jpeg
Details
OK Case.
(deleted), text/html
Details
One of the source files of NG cases. Width of img element is smaller than actual image width and bottom line is cut off with recent Firefox browsers.
(deleted), text/html
Details
Another source file of NG cases. Width of img element is smaller than actual image width and right edge line is cut off with recent Firefox browsers.
(deleted), text/html
Details
Captured image of firefox for file2_w203.html
(deleted), image/png
Details
Captured image of firefox for file3_w205.html
(deleted), image/png
Details
Captured image reproduced with online-browser-simulator.
(deleted), image/jpeg
Details
Result for IE8, which has no problem.
(deleted), image/jpeg
Details
Result for google Chrome, which has no problem.
(deleted), image/jpeg
Details
NG on windows7+firefox24
(deleted), image/png
Details
Add reftests
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Attached image jpg image with 1px blue border (obsolete) (deleted) — 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

1. prepare a image file with 1px colored border.
2. write a html file with the image.
3. display the html file in Firefox browser.

Try the following:
1.download the attached file; the jpg-image is 234 px in width and 60 px in height jpg.
2-1. write html like this:
<body>
<div>
<img width="234" alt="sample image" src="box_image.jpg">
</div>
</body>
2-2. write html like this:
<body>
<div>
<img width="203" alt="sample image" src="box_image.jpg">
</div>
</body>
2-3. write html like this:
<body>
<div>
<img width="205" alt="sample image" src="box_image.jpg">
</div>
</body>





Actual results:

4. Edges of the image are cut-off when the image are forced to resize with width attribute.

Note:
The test case result in:
2-1: OK
2-2: bottom edge is cut off
2-3: right edge is cut off

Note:
This issue doesn't happen always! For example, it may be OK when you mixed the all 2-1,2-2,2-3 code in same body element.
But actually it happens in some cases. One of the actual case is that images lay outed in 3*3 dimension using li element with float attribute, though I couldn't attach the site URL, sorry.  


Expected results:

The Expected results is display as same as in IE, google Chrome i.e. the size attribute DOESN'T cut off image edges.
Component: Untriaged → General
Component: General → DOM: Core & HTML
Product: Firefox → Core
Version: 24 Branch → unspecified
This sounds like a painting rounding issue: if you have a 1px border and you downscale, the 1px has to become <1px, which makes it rather hard to paint....
Component: DOM: Core & HTML → Graphics
(In reply to kjhgfnbmbjgiyut from comment #0)
> One of the actual case is that images
> lay outed in 3*3 dimension using li element with float attribute, though I
> couldn't attach the site URL, sorry.  

As you attached your picture, can you attach a fully working testcase which reproduces the issue, please.
I failed to repro it with only case #2.
Flags: needinfo?(kjhgfnbmbjgiyut)
Keywords: testcase-wanted
Version: unspecified → 24 Branch
(In reply to On vacation Oct 12 - Oct 27 from comment #1)
Thanks for changing categories.
As to 1px issue, ie and chrome displays fine. So I have reported this issue here.

(In reply to Loic from comment #2)
> As you attached your picture, can you attach a fully working testcase which
> reproduces the issue, please.
> I failed to repro it with only case #2.
I have an updated information. I also failed to repro case2-2, 2-3 on another environment;on the same firefox version(v.24) in MacOSX.
So I'll attach captured images of 2-2&2-3 back on the first environment in a few days.
Flags: needinfo?(kjhgfnbmbjgiyut)
We need a testcase, just attach HTML file (including your image stored on BMO) to reproduce the issue.
You can attach some screenshots if you want too.

        Attachment #815704 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        OK Case. (deleted)
        — 
      

    


  

    
    

    
  


  

    
    

    
  


  

    
    

    
  
Comment on attachment 817031 [details]
Captured image reproduced with online-browser-simulator.

The issue has reproduced with online-browser-simulator.
Upload file2_w203.html on any server you own, and sign in https://saucelabs.com/, and test the file with parameters like this: WindowsXP firefox24 1024x768.

    
    

    
  

      
      
      
      

        Attached image
          
        Result for IE8, which has no problem. (deleted)
        — 
      

    


  

    
    

    
  


  

    
    

    
  
I tested again on WindowsXP, and it causes problem with later version than firefox 18, as listed below:
_____________________
firefox 24.0 NG
firefox 22.0 NG
firefox 20.0.1 NG
forefox 19.0.2 NG
_____________________
forefox 18.0.2  OK
forefox 17.0 ESR  OK
forefox 16.0.2  OK
_____________________

With firefox 19.0.2 or laters, bottom line is cut-off when you display file2_w203.html(see attached files), and right edge is cut-off when you display file2_w205.html(see attached files).
See attached files :file4_w203_NG.png and file5_w205_NG.png.

This has reproduced with online-browser-simulator.
Upload file2_w203.html on any server you own, and sign in https://saucelabs.com/, and test the file with parameters like this: WindowsXP firefox24 1024x768.
The result I had is also attached as file6_w203WithBrowserEmulator.jpg.

For reference, I also attach the result for IE8(file7_withIE8.jpg) and chrome ver.28.0(file8_withChrome.jpg) with file2_w203.html.

All test are done with zoom 100%(no zoom).

    
    

    
  

      
      
      
      

        Attached image
          
        NG on windows7+firefox24 (deleted)
        — 
      

    


  
I did an additional test on windows7+firefox24 in online simulator. It also fails. Both bottom and right edges are cut off.

    
    

    
  
Comment on attachment 817124 [details]
NG on windows7+firefox24

It has a problem.

    
    

    
  
Simple testcase:
data:text/html;charset=utf-8,<img width%3D"203" src%3D"https%3A%2F%2Fbug925611.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D817024">

The issue appears when the user plays with the zoom levels (zoom out). I tried with HWA on/off, same behavior.
In my case, I need to zoom out to make the issue appear (default is OK).
URL: data:text/html;charset=utf-8,<img wid...

    
    

    
  
Regression range:
good=2012-12-04
bad=2012-12-05
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6fa6e55a93b2&tochange=1942b4d64dc8

Suspected bug:
Joe Drew — Bug 804559 - Use correct rounding code instead of rolling our own. r=jrmuizel Drew's First Law of Programming: If you write your own rounding code, you will be wrong 100% of the time.

Screenshot: http://i.imgur.com/XHX4hNC.png

Alice, are you OK with the reg range?
Blocks: 804559
Status: UNCONFIRMED → NEW
Component: Graphics → ImageLib
Ever confirmed: true
Keywords: testcase-wanted → regression, 
          
            testcase
Version: 24 Branch → 20 Branch

    
    

    
  
Note the missing edge is visible, but with difficulty.

    
    

    
  
image.high_quality_downscaling.enabled = false helps

So, I think root cause is Bug 486918.
And Bug 804559 expose the problem.

    
    

    
  
data:text/html;charset=utf-8,<img width%3D"203" src%3D"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAOoAAAA8CAYAAABo8yZ8AAAGzElEQVR4nO2dTW9cVx2H7w4WlfIRksb2vMZpEyqBEOyqYo/dwpYdGwqKKlWR4+AQFizYUVVCLKiERBK7Sc2OBd8A8RVALaS2xy+NTWJ77Jk7c1/OeVicc1/sKFJnWilc9HukR/87Z67PyIvfPeeec6UbPP3mN/jHhVf46/QUf6zXuddosdq6wnq7ycOpKf483eST6RZrtSvcb7RYq9dZbdS532hyv9HkUa3OJzMzUsoxfVSbYW2mxmqtxmqtwZ/qTf7QbvP72Ta/u9rkt69d59dX3+RX135E8M8LF+Dvf4PHn8G/PoXtXdjYgO4mdLdgcwc296C7B9t7sL0N21uws+PsbkspJ7ZbHG/vwt4+PNmH/V04OIZ9ePfbPyb4y6WLLqQ7mzA8hTCEw6dwdACnx3DSh5MQBpmnzrDvzh2Eqqqqk9RBCP0BnA7hJIJ+DGEKUQpRDLE7fOP77xLca7fh35/BySGQMIxGgAFiZ2rcxxwDNqHcaFVVVceuANZaMBZSsAZGQOg9Bj5PofXDuwQfvTrlhtwwJEpiwtiQAif9HqkZuV5dPyTnfsRh/DdSynG1xFgSwGBxQR0AQ6AHfAE0F28TfHzlKmxtwdEzwrBPCkQYIjvyHbhwRt4Ugz0zxCbko6+U8ktriUmISIiwxBhiRqSEpAxJ6QFdA7Pv/IJgrTYNWxvQPyKJBwyjkDAZkDAkjA59h4kLLwmWES73xRUhlVJOrC2NsAmGCENCwhB4Bry2sESwXqvBxufQ74GNMDbCkhCbEyxhHtTE/zF5UGMf1IRUSjmBhoS0uE81FuMT5UZXOAKuzy8RrE813BZM79QtHOU3oX6I9p9Tb76YZPwik5VSTiS+GoqAldpTCz0Lb7x1i+DRVNPtk/YGefBcPy7bWV/Z4Hymcynl5JZDW2qzkAf1GLj+g1sEqzNt6O65/RxcqN3CkSHBrQCXJ7wJZy8I7oORUo7t2d2UbEB0i7YJI+ApcLXzPsGDWtM9ceSDmuRBTUhIzgR16I/dyi9FWF/6pUnKaprNVLM8ZbsrCQkD4ABoL75PsFqruccC+ye4xaEsiNkYagBKHWbbMe6HXvY0X8qqC7g42WKETXED41NgtnOL4EG95p7d7Z/gnjgCt0/qg2oTsK7DFEOxb1ps0kopJxP8gR9kLcW6Uh7U+RWCe/WGe7h+cEo2eroOfCD96m7RcRZWIcRXw2cp20WxxYiaQDH17ZSD2h+ALQfSn15KehFUU/yWLbdJKcfT58w/P39+6rsPtBZWCNZql/3Utwhq8ZigyfuzWduZMVsI8bVgi2L9QQQ8wT/r+7B2CbY34HRQGnpLz/PmQXVPIT0/wRZCfF0U961uRfgJUFdQhfjfQkEVogIoqEJUAAVViAqgoApRARRUISqAgipEBVBQhagACqoQFUBBFaICKKhCVAAFVYgKoKAKUQEUVCEqgIIqRAVQUIWoAAqqEBVAQRWiAiioQlQABVWICqCgClEBFFQhKoCCKkQFUFCFqAAKqhAVQEEVogIoqEJUAAVViAqgoApRAV4Y1Eczl6C74V676E9MzwfVQvEex1JbObCqqqpj1+fHveK9qdlrF+uLtwnWpy9Ddwv6w3MdnB1R3YtW4yKoebuUciIp3jCeApbsxcYxEDME9shG1Gn/IuPBkJQCN6qWO8468JRDiqqq6rjV4mIZgc+eKeUsZkA2oq4QrNYvw+4WDIdE5BNcRqUOXN/uHjX7Psto+Yqgqqo6Xs2OARcoY8C6kXUI7APNhTsvDmpEOekA5oVBlVJO5tlZq2807r5yhAtqq3PHT32ze9TSVNaWOzGUkl5uO/cjUsrJzPKUAqnLWgQcAO3OSnkxya36Fqu8FJ+zPzbm+Y5f9j8o5f+LxviQunlrFtTZTraPurMBgwEpDksxNOed+GTm7dl3gPXbOVLKL295K6bY/syO3arvAdBeuEnwsHYRdh5DeJrPmdPSn1qKoFrM2XYf0lRKObbng+vasvwZ+rhV39biTYL1xkXY+DQP6jAaYYHYGlJwt6UGsCnWpnlQjQVrX/Y1ScrqmmCJcTssQ292HAJHuH3U6fklggfNFnR34SjMT+wl0LMwAIYWRrbooO8NSx1KKSczy9Mp0CvVY9y0dxOYfvsuwYf178ETAwk8Hrkv94FdYNuf/B/vAfCFd5/iXCnl+B4Ah7hQHgLPzrmPC+rU4m8IPphd4OftRb7z3Z/QXLxFs7NEY2GJ+uJtGm/foTn/S1rzK7Tml2l2lqgtLFNbWKaxsOzPXZZSTmCrc5Nrczf41tzPuDZ3g9fnbvD63Huudn7Klc57vPrWXWbe+YD/AslaKFqu2mLsAAAAAElFTkSuQmCC">
Flags: needinfo?(jmuizelaar)
Blocks: 486918
OS: Windows XP → All
Hardware: x86 → All

    
    

    
  
Seth, maybe you can take a look?
Flags: needinfo?(seth)

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #1)
> This sounds like a painting rounding issue: if you have a 1px border and you
> downscale, the 1px has to become <1px, which makes it rather hard to
> paint....

With other images it seems that the scaler is cutting off the right pixel-column or the bottom pixel-row (or in rare cases both) and then resizes to the intended size.

For example: http://elbart.bplaced.net/scaling/

    
    

    
  


  
This is basically a rounding issue, but IMO the right fix is not to change the rounding algorithm again (as was done in bug 804559, fixing one bug and introducing another) but to just pass in the dest rect size from DrawImageInternal. We already need this information in VectorImage, so all that's really changing here is that RasterImage is taking of it too instead of trying to reproduce the calculation.

Note that we're actually not passing in a different value in the VectorImage case than we were before, although it seems like we are. We use the same code here that we did to compute aImageSize in DrawSingleImage.

        Attachment #8349793 -
        Flags: review?(tnikkel)

    
  
Assignee: nobody → seth
Status: NEW → ASSIGNED

    
  
Flags: needinfo?(seth)

    
  
Flags: needinfo?(jmuizelaar)

    
  

          status-firefox30:
          --- → ?

    
    

    
  
Just noticed this bug hasn't made any progress in a month. And now the try job is gone. =\

I'm going to rebase it, push a new try job, and try to get some progress on the review.

    
    

    
  
Comment on attachment 8349793 [details] [diff] [review]
Make sure Draw*Image and the HQ image scaler use the same final image size.

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>@@ -4273,18 +4273,26 @@ DrawImageInternal(nsRenderingContext*   
>+  // Compute the viewport size, which is the size of the destination rect in
>+  // dev pixels. In other words, this is the size that the image will appear
>+  // on the screen. The SVG spec uses this information as an input when
>+  // rendering. Raster images use it for high-quality scaling.
>+  nsIntSize viewportSize;
>+  viewportSize.width = NSAppUnitsToIntPixels(aDest.width, appUnitsPerDevPixel);
>+  viewportSize.height = NSAppUnitsToIntPixels(aDest.height, appUnitsPerDevPixel);

If there is a scale on the gfxContext here then this won't be the size of the image on the screen. ie for <img style="width: 1px; height: 1px; transform: scale(190,190);" src="a200x200image.png"> the viewport size will be 1x1 pixel, whereas we want it to be 190x190 pixels.

        Attachment #8349793 -
        Flags: review?(tnikkel) → review-

    
  

          status-firefox31:
          --- → ?

    
    

    
  
I have the feeling this bug has been made more obvious recently on Google-Images-result-pages when "Smooth scrolling" is disabled and you're scrolling down with the PgDown-key.

    
    

    
  
Showing how obvious this bug is: http://elbart.bplaced.net/scaling/scaling%20nightly%202014-04-05.mp4
Note the first three search-results after switching to the tab and the images in "Logo" and "Mobzilla" during every reload.

http://elbart.bplaced.net/scaling/scaling%20nightly%202014-04-05%20smoothscroll%20off.mp4
Flags: needinfo?(seth)

    
    

    
  
Yeah, this really needs to get landed. It needed to wait for some things things to land, unfortunately, but the time is right now.
Flags: needinfo?(seth)

    
    

    
  


  
Rebased, and with Timothy's review comments addressed.

I'll wait until try looks green before rerequesting review.

    
  

        Attachment #8349793 -
        Attachment is obsolete: true

    
    

    
  


  
Ack. Made a mistake in the previous version of the patch. Here's a better version.

    
  

        Attachment #8406625 -
        Attachment is obsolete: true

    
    

    
  


  
So the previous try jobs were testing something that I was a bit curious about - whether we could get away with scaling aViewportSize according to the context even for SVGs. I thought it was possible that we would, because we currently have an issue where we don't handle gfxContext scaling correctly in the SVG case. The reftest results make it pretty clear that it doesn't work out, though. That will need to be handled separately.

Now, I've taken a closer look at the code, which was pretty well swapped out for me since this patch has been sitting for so long. At this point I actually don't think that we *should* be taking the gfxContext scale into account, because the existing code doesn't do it - take a look at RasterImage::DrawWithPreDownscaleIfNeeded. The scale computation is purely based upon aUserSpaceToImageSpace, which does not take the gfxContext scale into account. (See nsLayoutUtils::ComputeSnappedImageDrawingParameters. If aUserSpaceToImageSpace *does* take the gfxContext scale into account, then the gfxContext matrix is reset to the identity matrix, so we can essentially view it as never taking the gfxContext scale as perceived by RasterImage into account.)

Now, I am open to the idea that we should be taking this into account after all, but I really think we should (1) have some test case demonstrating the problem, and (2) handle that issue as a separate bug. As far as I can tell, the behavior of this patch replicates exactly what the current code does, except with fewer bugs.

I have made one change to the patch compared to the previous version, and that is to remove the sentence described aViewportSize as representing the image's "size on the screen". That is true for most cases, but not all, and I agree that it could mislead the reader.

        Attachment #8406700 -
        Flags: review?(tnikkel)

    
  

        Attachment #8406630 -
        Attachment is obsolete: true

    
    

    
  
I've pushed another try job using the current patch. It should be identical to the original green try results, but since the code has changed around it, it's worth running again.

https://tbpl.mozilla.org/?tree=Try&rev=9d804191f90f

    
    

    
  
No, I don't think we should consider the transform on the context in RasterImage. I think that we need to take into account the transform on the context when we are calculating viewportSize in DrawImageInternal.

For the following example

<img style="width: 1px; height: 1px; transform: scale(190,190);" src="a200x200image.png">

I traced through the code with your patch applied. aViewportSize passed into RasterImage was 1x1, but this image is drawn on screen as 190x190. aDest the equivalent of 1x1 pixels but stored in app units, and the context will have a scale by 190 on it for the CSS transform. aUserSpaceToImageSpace's scale is approx 1.05 as expected.

    
    

    
  
Comment on attachment 8406700 [details] [diff] [review]
Make sure Draw*Image and the HQ image scaler use the same final image size

Talked with Seth on irc to explain my concern. We'll need a change here before this can land, so clearing review request for now.

        Attachment #8406700 -
        Flags: review?(tnikkel)

    
    

    
  


  
In the end, to reliably get the same scaling behavior, we didn't just need to account for gfxContext scaling - we also needed to use the same pixel snapping algorithm. This version of the patch is also more aggressive on the RasterImage size, as it completely eliminates the usage of |scale| to determine whether to use the HQ scaled image, and instead consults aViewportSize directly.

This version of the patch resolves the examples in this bug, as well as Timothy's example, at a variety of zoom levels and in both high-DPI and low-DPI mode. It passes a large subset of tests. I'll push a try job to test the whole set shortly.

Note that for now I'm calculating the viewport size for SVG images in the old way. There are some issues preventing this form working as desired that I don't want to fix in this bug, but I'll take care of them as part of some other SVG scaling bugs (for example, bug 942364) that I'm working on now.

        Attachment #8408727 -
        Flags: review?(tnikkel)

    
  

        Attachment #8406700 -
        Attachment is obsolete: true

    
    

    
  


  
Typo fix.

        Attachment #8408756 -
        Flags: review?(tnikkel)

    
  

        Attachment #8408727 -
        Attachment is obsolete: true

        Attachment #8408727 -
        Flags: review?(tnikkel)

    
    

    
  
Updated try job, just to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=f5ae91101410

    
    

    
  


  
Turns out that asserting something that isn't necessarily true is a bad idea!

Just removed the bad assert; the rest of the patch is unchanged.

        Attachment #8408806 -
        Flags: review?(tnikkel)

    
  

        Attachment #8408756 -
        Attachment is obsolete: true

        Attachment #8408756 -
        Flags: review?(tnikkel)

    
    

    
  
I hope this is the final try job I post for this patch. I want to be confident everything is green, though.

https://tbpl.mozilla.org/?tree=Try&rev=9d0c7c321190

    
    

    
  
So green. So green.

    
    

    
  
Comment on attachment 8408806 [details] [diff] [review]
Make sure Draw*Image and the HQ image scaler use the same final image size

>@@ -4764,16 +4770,25 @@ ComputeSnappedImageDrawingParameters(gfx
>+  gfxRect viewport = devPixelDest;
>+  if (didSnap) {
>+    // As above, we need to adjust the scale to compensate for the matrix reset.
>+    viewport.Scale(currentMatrix.xx, currentMatrix.yy);
>+  }
>+  aCtx->UserToDevicePixelSnapped(viewport, true);
>+  nsIntSize viewportSize(NSToIntCeil(viewport.width), NSToIntCeil(viewport.height));

Isn't this going to scale by the current matrix's scale, and then UserToDevicePixelSnapped is going to apply the current matrix (applying the current matrix scale again) and pixel snap? If so, that doesn't seem like what we want.
Flags: needinfo?(seth)

    
    

    
  
That's true. That code was causing the HQ scaling code to produce a scaled image that was too large. 

I think we don't want any call to |Scale| there at all, because devPixelDest doesn't get scaled by the gfxContext's scale here. As far as I can tell, |devPixelDest| is in the local coordinate space of the gfxContext. (That is, the gfxContext's matrix represents an additional transformation which is not captured in devPixelDest.)

That actually means that applying |UserToDevicePixelSnapped| is the right thing to do only when |didSnap| is true, because in that case we will reset the gfxContext's matrix to identity when we draw, but we want to maintain the scale it implied.

In cases where |didSnap| is false, I think we just need to compute |intDestSize| directly from |devPixelDest| and be done with it. The scale on the gfxMatrix will take care of the rest. We need to handle this situation in VectorImage/RasterImage anyway, because |imgIContainer::Draw| will have callers that don't go through |DrawImageInternal|.

I think this needs to be reworked a bit more, so I'm unsetting the review flag.
Flags: needinfo?(seth)

    
  

        Attachment #8408806 -
        Flags: review?(tnikkel)

    
    

    
  
Now I'm noticing the jump in the new newtab-page, which landed in 31, too.

    
    

    
  
Workaround-userstyle:

> img {
>   transform:rotate(0deg);
> }

    
  
Depends on: DrawAPIRefactor

    
  

        Attachment #8408806 -
        Attachment is obsolete: true

    
    

    
  
In the end this is being resolved by bug 1037713. We should land reftests for this, though, since this has proven to be an *excellent* test case for numerical accuracy issues within the image drawing pipeline.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add reftests (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This patch adds reftests for this issue. (Which is a little ugly since scaling
behavior differs between platforms; they have to be fairly fuzzy.) They're
marked as random, here, since after all they don't actually pass right now. The
reftest patch for bug 1043560 will remove the random annotation.

        Attachment #8463622 -
        Flags: review?(tnikkel)

    
    

    
  
Comment on attachment 8463622 [details] [diff] [review]
Add reftests

Have you done any testing to see if 50ms is long enough to get us the hq downscaled version a large enough precentage of the time?

        Attachment #8463622 -
        Flags: review?(tnikkel) → review+

    
    

    
  
(In reply to Timothy Nikkel (:tn) from comment #54)
> Have you done any testing to see if 50ms is long enough to get us the hq
> downscaled version a large enough precentage of the time?

I haven't personally, no, but I believe that Daniel did when he added the downscale-1 test. I just reused his JavaScript from that test.

I do see different failures for the tests with HQ downscaling disabled vs enabled, which strongly suggests that HQ downscaling has actually successfully completed on all the try runs I've done so far.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add reftests (deleted)
        — 
          — Splinter Review
      

    


  
Timothy, if you have a chance, could you look this over one more time? I totally
rewrote this because I got frustrated with the other approach (an image of a
rectangle with a 1px border), which ended up extremely fuzzy, with a different
fuzziness required on every major platform.

 The idea now is that each test just checks one edge of the original rectangle.
 I think this approach is much better, and the new tests are also *much* more
 thorough.  I've created tests for every downscaling zoom setting you can reach
 in Firefox via the browser UI. It was easy to do with the new approach because
 the reference is just about:blank instead of a custom image created for each
 test.

 The most important thing, for me, is that these tests unambiguously fail
 without bug 1043560 (although which specific tests fail varies by platform -
 hence the random annotation for now), and they unambiguously succeed with bug
 1043560 in place. I didn't feel comfortable with the old approach because even
 after bug 1043560, the tests were still quite fuzzy - it was impossible to
 avoid when using images as the references.

        Attachment #8465207 -
        Flags: review?(tnikkel)

    
  

        Attachment #8463622 -
        Attachment is obsolete: true

    
    

    
  
In case it's not clear, the (203,52) and (205,53) sized variants in these tests correspond to the previous downscale-2x and downscale-3x tests. Each size tends to produce different kinds of failures. I think it's worth having them both.

        Attachment #8465207 -
        Flags: review?(tnikkel) → review+

    
    

    
  
This was fixed by bug 1037713.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

    
    

    
  
Great work, thank you very much!

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: