Closed Bug 1312988 Opened 4 years ago Closed 4 years ago

Flash video is continuously flickering or a still image on certain site (NHK world On Demand & TV Live)

Categories

(Core :: Graphics: Layers, defect)

52 Branch
x86
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 - fixed

People

(Reporter: alice0775, Assigned: nical)

References

Details

(Keywords: regression)

Attachments

(5 files, 6 obsolete files)

5.29 KB, text/plain
Details
545 bytes, text/html
Details
4.96 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.86 KB, patch
sotaro
: review+
sotaro
: feedback+
Details | Diff | Splinter Review
4.79 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Attached file aboutSupport.txt
[Tracking Requested - why for this release]: Flash player does not work properly.

Bug 1311952 did not fix this problem. 

It is required to enable the Hardware Acceleration of Flash Player 23.0.0.205.

Reproducible: 100% with newly created profile

Steps To Reproduce #1:
0. Start new instance of Nightly52.0a1
1. Open http://www3.nhk.or.jp/nhkworld/en/live/
2. Wait to automatically playing the video(it is sound only until fix the Bug 1311952)
3. Open http://www3.nhk.or.jp/nhkworld/en/vod/ in the same tab
4. Click on [>] button in the Flash video
   ---- observe, Flash video is continuously flickering
5. Open again http://www3.nhk.or.jp/nhkworld/en/live/ in the same tab
   ---- observe, Flash video is continuously flickering


Steps To Reproduce #2:
0. Start new instance of Nightly52.0a1
1. Open http://www3.nhk.or.jp/nhkworld/en/vod/
2. Click on [>] button in the Flash video
3. Open http://www3.nhk.or.jp/nhkworld/en/live/ in the same tab
   ----  observe, Flash video is a still image and vibrated.

Actual Results:
Flash video is continuously flickering or a still image

Expected Results:
Playback properly

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=61da7056917a6361f5499f5431b83421979c5233&tochange=1ecca76e7b94b1356a67cae946d7d4a2a117f114

Regressed by: 1ecca76e7b94	Sotaro Ikeda — Bug 1305490 - Use ALLOC_UPDATE_FROM_SURFACE flag r=mattwoodrow
Flags: needinfo?(sotaro.ikeda.g)
Summary: Flash video is continuously flickering or a still image on certain site (NHK world On Demand) → Flash video is continuously flickering or a still image on certain site (NHK world On Demand & TV Live)
Assignee: nobody → sotaro.ikeda.g
I am not sure whether the attachment is correct testcase or not.
However, with this attachment, I got a same regression window about the flickering problem.
I succeeded to reproduce the flickering.
Flags: needinfo?(sotaro.ikeda.g)
The Problem seems to exist around gfx/layers.
Component: Plug-ins → Graphics: Layers
It seems related to mixed use of BufferTextureHost and DXGITextureHostD3D11

http://www3.nhk.or.jp/nhkworld/en/live/ uses DXGITextureHostD3D11 for rendering.
http://www3.nhk.or.jp/nhkworld/en/vod/ uses BufferTextureHost for rendering.
Both BufferTextureHost and DXGITextureHostD3D11 uses DataTextureSourceD3D11 as DataTextureSource. BufferTextureHost's one could be reused by other TextureHosts. But DXGITextureHostD3D11's one could not be used by other TextureHosts.
TextureClients allocation with RecycleAllocator also has a problem. Same RecycleAllocator is used for both RecvShowDirectBitmap() and RecvShowDirectDXGISurface(). Then a TextureClient in RecvShowDirectBitmap() could be reused in RecvShowDirectDXGISurface() later.
Attachment #8805015 - Flags: review?(nical.bugzilla)
Attachment #8805016 - Flags: review?(nical.bugzilla)
Attachment #8805016 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8805015 [details] [diff] [review]
patch part 1 - Check if DataTextureSource is reusable by other TextureHosts

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

It would be simpler to fix this by having DXGITextureHostD3D11 call SetOwner(this) on its TextureSource. This will prevent BufferTextureHost from recycling the texture, without requiring additional state. This TextureSource recycling mechanism is also used by other texture types that bind shared handles to the same texture id (gralloc for example), so having IsReusableByOtherTextureHosts return false for them would be confusing.

Side note: it would be even better if the DXGI texture host used a TextureSource that was not a DataTextureSource since it is not intended to be used as such (and ends up causing issues). Yet another example of poor usage of class inheritance as a cheap way to reuse code (which was totally my fault, I think I wrote this originally).
Attachment #8805015 - Flags: review?(nical.bugzilla)
Thanks for the comment! I am going to update the patch.
(In reply to Nicolas Silva [:nical] from comment #9)
> Comment on attachment 8805015 [details] [diff] [review]
> patch part 1 - Check if DataTextureSource is reusable by other TextureHosts
> 
> Review of attachment 8805015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be simpler to fix this by having DXGITextureHostD3D11 call
> SetOwner(this) on its TextureSource. This will prevent BufferTextureHost
> from recycling the texture, without requiring additional state. This
> TextureSource recycling mechanism is also used by other texture types that
> bind shared handles to the same texture id (gralloc for example), so having
> IsReusableByOtherTextureHosts return false for them would be confusing.

SetOwner(this) seems simpler in this bug. But we need to override SetOwner() to handle dataSource->SetOwner(nullptr) in ImageHost::SetCurrentTextureHost().
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp#121
Attachment #8805398 - Flags: review?(nical.bugzilla)
Attachment #8805398 - Flags: review?(nical.bugzilla)
Make as less error prone.
Attachment #8805398 - Attachment is obsolete: true
Attached patch patch_1312988_p1_4 (obsolete) — Splinter Review
Attachment #8805880 - Attachment is obsolete: true
Attachment #8805883 - Flags: review?(nical.bugzilla)
This patch is similar in principle to your patch, except that it is phrased in terms of whether the texture source can perform texture uploads instead of ownership semantics. The fact that DXGITextureHosts create DataTextureSources but expects that we don't use them as such is really the root of the problem. So I find it easier to understand the solution if it refers directly to the problem rather than complicating the ownership semantics (sorry for pointing you in that direction, I naively thought it would be a one-liner).

What do you think about this approach?

(It should work but I didn't even build it on Windows)
Attachment #8805947 - Flags: feedback?(sotaro.ikeda.g)
Tracking 52+ for this regression which affects Windows.
Attachment #8805883 - Attachment is obsolete: true
Attachment #8805883 - Flags: review?(nical.bugzilla)
Comment on attachment 8805947 [details] [diff] [review]
Counter-proposal: ensure that DataTextureSourceD3D11 is not used for for texture uploads if it was created around a DXGI handle

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

It worked and simper solution! We also needs same thing to DataTextureSourceD3D9.
Attachment #8805947 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Assign to :nical, since he is working for the bug.
Assignee: sotaro.ikeda.g → nical.bugzilla
Comment on attachment 8805947 [details] [diff] [review]
Counter-proposal: ensure that DataTextureSourceD3D11 is not used for for texture uploads if it was created around a DXGI handle

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

Tried the patch out and it seems to be ready for review in its current state.
Attachment #8805947 - Flags: review?(sotaro.ikeda.g)
Attachment #8805947 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8807471 - Flags: review?(sotaro.ikeda.g)
Attachment #8807471 - Flags: review?(sotaro.ikeda.g) → review+
Depends on: 1317131
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5259f662a65
Separate the recycling allocators for gpu and cpu textures. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/16cf03e36df3
Prevent D3D9 DXGI TextureSource to be used as a DataTextureSource. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/42b9ad84b621
Prevent D3D11 DXGI TextureSource to be used as a DataTextureSource. r=sotaro
tracking 53- as this is resolved fixed.
I can reproduce the problem on Nightly 2016-Nov-18 build[1].
I can verify to fix the problem on Nightly 2016-Nov-22 build[2].

[1]
https://hg.mozilla.org/mozilla-central/rev/28e2a6dde76ab6ad4464a3662df1bd57af04398a
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161118030222

[2]
https://hg.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161122030216


Please up lift this to Aurora52.0a2.
@:nical
Please uplift this to Aurora52.0a2.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8805016 [details] [diff] [review]
patch part 2 - Split RecycleAllocator

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Flash video flickers for some users on widnows
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Low, needs the three patches but they don't do risky things.
[String/UUID change made/needed]: None.
Flags: needinfo?(nical.bugzilla)
Attachment #8805016 - Flags: approval-mozilla-aurora?
Comment on attachment 8805947 [details] [diff] [review]
Counter-proposal: ensure that DataTextureSourceD3D11 is not used for for texture uploads if it was created around a DXGI handle

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Flash video flickers for some users on widnows
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Low, needs the three patches but they don't do risky things.
[String/UUID change made/needed]: None.
Attachment #8805947 - Flags: approval-mozilla-aurora?
Comment on attachment 8807471 [details] [diff] [review]
Same patch for d3d9 textures

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Flash video flickers for some users on widnows
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Low, needs the three patches but they don't do risky things.
[String/UUID change made/needed]: None.
Attachment #8807471 - Flags: approval-mozilla-aurora?
Comment on attachment 8805016 [details] [diff] [review]
patch part 2 - Split RecycleAllocator

part of fix for video issue, take in aurora52
Attachment #8805016 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8805947 [details] [diff] [review]
Counter-proposal: ensure that DataTextureSourceD3D11 is not used for for texture uploads if it was created around a DXGI handle

part of fix for video issue, take in aurora52
Attachment #8805947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8807471 [details] [diff] [review]
Same patch for d3d9 textures

part of fix for video issue, take in aurora52
Attachment #8807471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1372870
You need to log in before you can comment on or make changes to this bug.