1221840 - 1px horizontal black lines on CSS background-image (Youtube HTML5 video)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Petes]

Attachment: Clipboard01.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Play an HTML5 video on Youtube.


Actual results:

I see a black horizontal 1px line on the lower part (above the progress bar), when in fullscreen I see two horizontal lines, one on the upper part of the screen (aprox 20% under the very top) and the same one above the progress bar, both lines about 1px thick.


Expected results:

No black lines should be on top of all the videos.

Comment 1

[Petes]

Attachment: Clipboard02.png

Comment 2

[Loic]

Are you able to reproduce the issue:
1) in safe mode: https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
2) with a fresh profile: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

In addition, type about:support in the location bar and copy here the section "graphics".

Comment 3

[Alice0775 White]

seems dup of Bug 1221169

Comment 4

[Petes]

In safe mode the problem dissapears (the video is extremely slow though).
With a fresh profile the lines are still there.

The problem is also happening on my laptop so I believe it may be either configuration going through sync or a generalized problem.

One thing I noticed is that when I zoom in the page (a youtube video page) for example from 100% to 110% everything gets bigger as it should but when I zoom from 110% to 120% the page actually gets smaller and there is where the lines appear. If I zoom even more, the lines will stay there and grow respective to the zoom level.

Comment 5

[Petes]

Gráficos
Asynchronous Pan/Zoom	ninguna
Descripción del adaptador	ATI Radeon HD 4600 Series (Microsoft Corporation - WDDM v1.1)
Direct2D habilitado	true
DirectWrite habilitado	true (6.3.9600.17999)
Drivers del adaptador	aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
Fecha del driver	6-20-2012
GPU #2 Activa	false
ID de dispositivo	0x9490
ID de fabricante	0x1002
ID de subsistema	20091787
Procesador WebGL	Google Inc. -- ANGLE (ATI Radeon HD 4600 Series (Microsoft Corporation - WDDM v1.1) Direct3D11 vs_4_1 ps_4_1)
RAM del adaptador	1024
Soporta decodificación H264 por Hardware	true
Ventanas con aceleración gráfica	1/1 Direct3D 11 (OMTC)
Versión del driver	8.97.10.6
windowLayerManagerRemote	true
AzureCanvasBackend	direct2d 1.1
AzureContentBackend	direct2d 1.1
AzureFallbackCanvasBackend	cairo
AzureSkiaAccelerated	0

Comment 7

[Loic]

Could you update your ATI drivers please, as it disappears in safe mode, it's surely related to hardware acceleration.
http://www2.ati.com/drivers/legacy/13-1-legacy_vista_win7_win8_64_dd_ccc.exe (catalyst 13.1)

Comment 8

[Alice0775 White]

Thanks Petes, I can reproduce this now.



Steps to reproduce:
1. Open https://www.youtube.com/watch?v=WKB0JUkksJg
2. Zoom in (in my case 170%)
3. Mouse hover the video to show video controls 


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b39c0e12d03&tochange=253c0c166893

Regressed by:
	db744be4dc3d	Mason Chang [:mchang] — Bug 1073209 - Eliminate usage of CreateSamplingRestrictedDrawable on d2d backends. r=jrmuizel

(FYI, Bug 1211264 did not fix this.)

Comment 9

[Alice0775 White]

Attachment: testcase

Comment 10

[Alice0775 White]

Attachment: Bug 1221840 1px line.html

Comment 12

[Alice0775 White]

[Tracking Requested - why for this release]: visual glitch on youtube and other page using background-image

Comment 13

[Julien L.]

Hello,
I have this bug with my Intel HD graphics 4600 and the driver dating back to march 2015.
And I experience the same thing with another laptop with AMD radeon graphics.

This is bug of the hardware acceleration, right?
Firefox 41 didn't have this bug, I don't know why Firefox 42 got this issue.

Comment 14

[Petes]

(In reply to Julien from comment #13)
> Hello,
> I have this bug with my Intel HD graphics 4600 and the driver dating back to
> march 2015.
> And I experience the same thing with another laptop with AMD radeon graphics.
> 
> This is bug of the hardware acceleration, right?
> Firefox 41 didn't have this bug, I don't know why Firefox 42 got this issue.

No Julien, it's a generalized problem, they will fix it.

Comment 15

[Mason Chang [Inactive] [:mchang]]

(In reply to Alice0775 White from comment #10)
> Created attachment 8684521 [details]
> Bug 1221840 1px line.html

Thanks a lot for this test case. For me, both Chrome and IE also display the one line through the content for this test case. Is this happening for you? Thanks!

Comment 16

[Mason Chang [Inactive] [:mchang]]

(In reply to Petes from comment #0)
> Created attachment 8683419 [details]
> Clipboard01.png
> 
> User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101
> Firefox/42.0
> Build ID: 20151029151421
> 
> Steps to reproduce:
> 
> Play an HTML5 video on Youtube.
> 
> 
> Actual results:
> 
> I see a black horizontal 1px line on the lower part (above the progress
> bar), when in fullscreen I see two horizontal lines, one on the upper part
> of the screen (aprox 20% under the very top) and the same one above the
> progress bar, both lines about 1px thick.
> 
> 
> Expected results:
> 
> No black lines should be on top of all the videos.

Thanks for filing this bug. Does this happen at all zoom levels for you or only specific ones? You can zoom in / out by holding control key and pushing + or - to zoom in / out. To reset your zoom level, you can push control and 0.

If you can only see the lines during certain zoom levels, can you please say how many times you zoomed in / out from the normal zoom level? Thanks!

Comment 17

[Alice0775 White]

(In reply to Mason Chang [:mchang] from comment #15)
> (In reply to Alice0775 White from comment #10)
> > Created attachment 8684521 [details]
> > Bug 1221840 1px line.html
> 
> Thanks a lot for this test case. For me, both Chrome and IE also display the
> one line through the content for this test case. Is this happening for you?
> Thanks!

With the testcase,
Firefox41 and IE11 does not display the line with any zoom level with/without HWA.
Nightly45.0a1 display the line with more than 80% zoom level. (Of course, it does not happen without HWA.)
Chrome48 display the line with any zoom level(except 125%). (Howecer, it does not happen on youtube.)

Comment 18

[Julien L.]

I see the lines during zoom levels from 90 % to 300 %
At 80% it's too small I can't see them.
By the way in fullscreen mode when there are two lines they come closer to the center of the screen when I zoom in.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: Test case without zoom

Another version of Alice's test case that works without a scale. All three browsers, IE 11, Chrome, and Firefox show a line through content. A local build of firefox with CSRD enabled does not show the line. Interestingly, Chrome shows the line for a split second, then it goes away. 

Chrome and IE 11 do not show the line during youtube specifically. Jeff, maybe we can do CSRD only when a video is playing? Do you have any idea what's going on with Chrome and the magically sometimes disappearing line?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #19)
> Created attachment 8684999 [details]
> Test case without zoom
> 
> Another version of Alice's test case that works without a scale. All three
> browsers, IE 11, Chrome, and Firefox show a line through content. A local
> build of firefox with CSRD enabled does not show the line. Interestingly,
> Chrome shows the line for a split second, then it goes away. 
> 
> Chrome and IE 11 do not show the line during youtube specifically. Jeff,
> maybe we can do CSRD only when a video is playing? Do you have any idea
> what's going on with Chrome and the magically sometimes disappearing line?

Your test case is missing a background-repeat: repeat-x. That's what makes the line disappear in IE11. We can fix this by plumbing through the repeat directions.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression, tracking.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Support repeating images in 1 Axis

Just seeing if this is roughly right. Carry the background-repeat information from the layer at [1], carry it down as a param until we can create the ImageRegion[2]. Then use the ExtendMode from the ImageRegion in gfxUtils::DrawPixelSnapped and add support in the backends to repeat in only 1 axis.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?from=nsCSSRendering.cpp#3027
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?from=nsLayoutUtils.cpp#6393

Comment 23

[Petes]

(In reply to Mason Chang [:mchang] from comment #16)
> (In reply to Petes from comment #0)
> 
> Thanks for filing this bug. Does this happen at all zoom levels for you or
> only specific ones? You can zoom in / out by holding control key and pushing
> + or - to zoom in / out. To reset your zoom level, you can push control and
> 0.
> 
> If you can only see the lines during certain zoom levels, can you please say
> how many times you zoomed in / out from the normal zoom level? Thanks!

I checked again and it looks like it actually appears at 110% and up, but the lines can be hard to see at 110%, at 100% or less I don't see any lines. 

The zoom levels do seem a little weird because when I zoom down the video can get bigger some times (though the text does get smaller), similarly when zooming up but I blame Youtube for that.

Comment 24

[Petes]

My monitor is 1080p BTW.

Comment 25

[Mason Chang [Inactive] [:mchang]]

Attachment: Support repeating images in 1 axis

Cleans up the aRepeat param in gfxDrawable at [1]. We just use the ExtendMode instead. Adds support in the d2d and d2d1.1 backends. I think this is mostly the extent of the work other than finishing the support in CG / Cairo / Skia backends. Is this roughly the right structure? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxDrawable.h#37

Comment 26

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8686778 [details] [diff] [review]
Support repeating images in 1 axis

Review of attachment 8686778 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me!

::: gfx/2d/Types.h
@@ +210,1 @@
>    REFLECT

Nit: Why not comment the other values while you're at it? =)

::: gfx/thebes/gfxDrawable.cpp
@@ +139,5 @@
>                            const Filter& aFilter,
>                            gfxFloat aOpacity,
>                            const gfxMatrix& aTransform)
>  {
> +    if (((aExtendMode != ExtendMode::CLAMP) || aOpacity != 1.0) && !mSurfaceDrawable) {

Nit: no need for the extra parentheses.

::: image/ImageRegion.h
@@ +134,4 @@
>      return Create(mRect + aPt);
>    }
>  
> +  void SetExtendMode(gfx::ExtendMode aExtendMode)

Please just pass the ExtendMode in via the Create*() factory methods and set it in the constructor. It seems like many callers can safely default to ExtendMode::CLAMP, so you can make it a default argument.

I don't see any cases where we need to change the ExtendMode after construction, so you can probably just remove SetExtendMode() altogether.

::: image/imgFrame.cpp
@@ +592,1 @@
>    ImageRegion region(aRegion);

Because SurfaceForDrawing() mutates |region|; it couldn't do that for |aRegion|, since |aRegion| is const.

This code could, uh, use some refactoring.

::: layout/base/nsCSSRendering.h
@@ +298,4 @@
>    DrawResult                mPrepareResult;
>    nsSize                    mSize; // unscaled size of the image, in app units
>    uint32_t                  mFlags;
> +  mozilla::gfx::ExtendMode  mExtendMode;

Looks like you need to initialize this in the constructor...

Comment 27

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #26)
> Because SurfaceForDrawing() mutates |region|; it couldn't do that for
> |aRegion|, since |aRegion| is const.

To be clear, I'm addressing the question you asked in your comment here. Unfortunately Splinter didn't include the question in the output. =)

Comment 29

[mxh]

Not fixed. Do it fast before you lose further users to Chrome. And try to fix the problem instead of marking duplicates.

Comment 30

[mxh]

Attachment: BUG

FF 42.0
Geforce 650M with latest driver

Comment 31

[Mason Chang [Inactive] [:mchang]]

(In reply to mxh from comment #29)
> Not fixed. Do it fast before you lose further users to Chrome. And try to
> fix the problem instead of marking duplicates.

We know it's not fixed. The original bug that was duplicated is this bug and we're trying to fix it. It's just the mechanics of how we use bugzilla.

Comment 32

[Mason Chang [Inactive] [:mchang]]

Attachment: Support repeating images in 1 axis

Includes the feedback from comment 26 and support across all the backends other than Cairo. Cairo doesn't support single axis repeating :(.

Try looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f363a98f486

Comment 33

[mxh]

I find it amazing that basic websites which almost 100% users visit are not tested before releasing a final version of firefox? I tried switching off hardware acceleration but then the performance is so poor that i have switched to chrome till the bug is resolved.

Comment 34

[Seth Fowler [:seth] [:s2h]]

(In reply to mxh from comment #33)
> I find it amazing that basic websites which almost 100% users visit are not
> tested before releasing a final version of firefox? I tried switching off
> hardware acceleration but then the performance is so poor that i have
> switched to chrome till the bug is resolved.

They're tested as thoroughly as we can, but in a piece of software this complex, and especially with graphics problems which can be hardware dependent, there are sometimes problems we don't discover until after the release. I'm sorry you ran into this problem; please understand that we're trying to fix the problem as quickly as possible, but these things are complex and take some time.

Comment 35

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8688186 [details] [diff] [review]
Support repeating images in 1 axis

Review of attachment 8688186 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good Mason! Nothing but minor style nits remain.

::: gfx/2d/DrawTargetD2D1.cpp
@@ +1484,5 @@
>        RefPtr<ID2D1Bitmap> bitmap;
>        image->QueryInterface((ID2D1Bitmap**)getter_AddRefs(bitmap));
>        if (bitmap) {
> +        /**
> +         * CREATE THE BRUSH WITH THE PROPER REPEAT MODES

Nit: Is this the part of the code where ALL CAPS is in vogue? =)

@@ +1517,5 @@
>        samplingBounds = D2D1::RectF(0, 0, pat->mSamplingRect.width, pat->mSamplingRect.height);
>      }
> +
> +    /**
> +     * Probably have to do same thing here again

Nit: "Probably" is not exactly confidence-inducing to a reviewer. =) I'm going to r+ because this looks right, but please double-check and remove this comment.

::: gfx/2d/Types.h
@@ +201,5 @@
>    OP_LUMINOSITY,
>    OP_COUNT
>  };
>  
> +enum Axis : int8_t {

Shouldn't this be |enum class|? I actually wouldn't have expected this to compile...

::: image/ImageRegion.h
@@ +163,5 @@
>  
>    gfxRect mRect;
>    gfxRect mRestriction;
>    bool    mIsRestricted;
> +  ExtendMode mExtendMode;

Maybe just pop this up above |mIsRestricted| so if we ever get another bool field here and want to turn them into bitfields, we can easily do so.

::: layout/base/nsCSSRendering.cpp
@@ +3344,5 @@
>    if (repeatY == NS_STYLE_BG_REPEAT_REPEAT) {
>      state.mFillArea.y = bgClipRect.y;
>      state.mFillArea.height = bgClipRect.height;
> +
> +    if (repeatMode == ExtendMode::REPEAT_X) {

I feel like this code is simple but strikes me as something that someone might misunderstand if they don't pay attention; maybe add a comment like "We're repeating on the Y axis; if we already determined that we're repeating on the X axis too, we need to use ExtendMode::REPEAT." Feel free to reword that; just giving an idea of what I think might help.

::: layout/base/nsLayoutUtils.h
@@ +1770,5 @@
>     *   @param aAnchor           A point in aFill which we will ensure is
>     *                            pixel-aligned in the output.
>     *   @param aDirty            Pixels outside this area may be skipped.
>     *   @param aImageFlags       Image flags of the imgIContainer::FLAG_* variety
> +   *   @param aExtendMode       How to extend the image over the dest rect

Nit: Maybe put a period at the end here?

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cae5c087063d

Comment 37

[Mason Chang [Inactive] [:mchang]]

Attachment: Support repeating images in 1 axis

Carrying r+, updated with feedback from comment 35.

Successful try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb906ad3ff11

Comment 38

[Mason Chang [Inactive] [:mchang]]

If lots of regressions come out of this, please feel free to back it out. I'll be on PTO until Monday, November 30th.

Comment 39

[Julien L.]

The bug seems to being fixed by Youtube. Now I see no lines at the bottom or at the top of the video when the controls are visible.

Comment 40

[mxh]

Bug is fixed for me too, dunno if youtube changed something or it was the new graphics driver (Nvidia v359.00) I installed

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cae5c087063d

Comment 42

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Any plans to uplift this patch to Aurora or even Beta?

Comment 43

[Mason Chang [Inactive] [:mchang]]

(In reply to Virtual_ManPL [:Virtual] from comment #42)
> Any plans to uplift this patch to Aurora or even Beta?

Sorry, no, this is a somewhat risky patch and it seems like youtube fixed it on their side from comment 39.