Closed Bug 1430610 Opened 2 years ago Closed 2 years ago

OMTV causes display list reconstruction with WebRender

Categories

(Core :: Graphics: WebRender, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mstange, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

With WebRender, off-main-thread video isn't keeping the main thread as idle as it should be. We cause the display list to be rebuilt all the time, from this stack:

    mozilla::PresShell::ScheduleViewManagerFlush(nsIPresShell::PaintType)
    InvalidateFrameInternal(nsIFrame*, bool)
    nsIFrame::InvalidateLayer(DisplayItemType, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*, nsRect const*, unsigned int)
    mozilla::dom::HTMLMediaElement::Invalidate(bool, mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> >&, bool)
    mozilla::VideoFrameContainer::Invalidate()
    mozilla::MediaDecoder::UpdateLogicalPositionInternal()
    mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher::DoNotify()
    mozilla::detail::RunnableMethodImpl<mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher*, void (mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher::*)(), true, (mozilla::RunnableKind)0>::Run()
    mozilla::AutoTaskDispatcher::DrainDirectTasks()
    mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run()
    mozilla::EventTargetWrapper::Runner::Run()
    mozilla::SchedulerGroup::Runnable::Run()
    nsThread::ProcessNextEvent(bool, bool*)

Without WebRender, nsIFrame::InvalidateLayer has a fast path where it returns early if the frame has a layer that "supports async update":

  // If the layer is being updated asynchronously, and it's being forwarded
  // to a compositor, then we don't need to invalidate.
  if ((aFlags & UPDATE_IS_ASYNC) && layer && layer->SupportsAsyncUpdate()) {
    return layer;
  }

We're not hitting that fast path and are calling SchedulePaintInternal later down in the function.
Based on Bug 1420430 Comment 4, I will take it for further study.
Assignee: nobody → vliu
(In reply to Vincent Liu[:vliu] from comment #1)
> Based on Bug 1420430 Comment 4, I will take it for further study.

After discussed with Jerry, it lacks of dlbi with wr enabled to be cause the rebuild all the time. I would untake it because it still has some discussions for this.
Assignee: vliu → nobody
(In reply to Markus Stange [:mstange] from comment #0)
> 
> Without WebRender, nsIFrame::InvalidateLayer has a fast path where it
> returns early if the frame has a layer that "supports async update":
> 

Ah, It seems like a regression of layer free with WebRender. When WebRender uses layers Bug 1359993 reduced the invalidation.
Comment on attachment 8942839 [details] [diff] [review]
patch - Change nsIFrame::InvalidateLayer() as to check WebRender's async update

mstange, can you feedback to the patch?
Attachment #8942839 - Flags: feedback?(mstange)
Comment on attachment 8942839 [details] [diff] [review]
patch - Change nsIFrame::InvalidateLayer() as to check WebRender's async update

This patch seems to address the problem. Thanks!
Attachment #8942839 - Flags: feedback?(mstange) → feedback+
(In reply to Vincent Liu[:vliu] from comment #2)
> (In reply to Vincent Liu[:vliu] from comment #1)
> > Based on Bug 1420430 Comment 4, I will take it for further study.
> 
> After discussed with Jerry, it lacks of dlbi with wr enabled to be cause the
> rebuild all the time. I would untake it because it still has some
> discussions for this.

I am not sure why this bug is related to dlbi. We do not need to think about if  nsIFrame::InvalidateLayer() is called with UPDATE_IS_ASYNC and WebRenderImageData for video already has async image pipeline.
Assignee: nobody → sotaro.ikeda.g
See Also: → 1420430
Blocks: 1420430
See Also: 1420430
Blocks: 1430451
Attachment #8943088 - Flags: review?(nical.bugzilla)
Comment on attachment 8943088 [details] [diff] [review]
patch - Change nsIFrame::InvalidateLayer() as to check WebRender's async update

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

I don't know this well enough to give a proper review, redirecting to Morris.
Attachment #8943088 - Flags: review?(nical.bugzilla) → review?(mtseng)
Comment on attachment 8943088 [details] [diff] [review]
patch - Change nsIFrame::InvalidateLayer() as to check WebRender's async update

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

LGTM
Attachment #8943088 - Flags: review?(mtseng) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dac05eadb245
Change nsIFrame::InvalidateLayer() as to check WebRender's async update r=mtseng
https://hg.mozilla.org/mozilla-central/rev/dac05eadb245
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.