Closed Bug 1245400 Opened 4 years ago Closed Last year

Add ability to know on why a frames was not composited.

Categories

(Core :: Graphics: Layers, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jya, Assigned: jya, NeedInfo)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 1 obsolete file)

Didn't know composited was a verb, but it appears to be used that way :)

There are currently multiple reasons a video frame may not be composited; this includes:
1- The media element is hidden
2- The frame is too much in the past and is dropped from the image container
(and probably more)

We can't at present distinguish between those two cases.
This is the cause for bug 1237160.

We need to be able to tell the difference so that we only report frames being dropped because the machine was too slow to paint them
Do you plan to work on this?
Flags: needinfo?(jyavenard)
No.

Would be great if you could thank you
Flags: needinfo?(jyavenard)
Severity: normal → enhancement
Whiteboard: [gfx-noted]
This shouldn't be an enhancement. It is critical to provide accurate frames dropped statistics. Right now we had to disable the feature and various web site, in particular YouTube will as such make the wrong decision and continue to serve too high quality video, even if the machine can't keep up
Severity: enhancement → normal
Comment on attachment 8923326 [details] [diff] [review]
Provide a way to get the dropped frame count when the element is visible for rendering.

IIUC, it should not be able to know if the element is visible or not while composition in ImageHost.
This idea came up to me, but I'm not sure if it's appropriate or not.

Hi Jeff, any suggestions ?
Attachment #8923326 - Attachment description: Provide waCount the dropped frame → Provide a way to get the dropped frame count when the element is visible for rendering.
Flags: needinfo?(jmuizelaar)
Attachment #8923326 - Attachment is patch: true
Attachment #8923326 - Attachment mime type: text/x-log → text/plain
This isn't an area I'm super familiar with but the approach seems reasonable.
Flags: needinfo?(jmuizelaar)
Comment on attachment 8923326 [details] [diff] [review]
Provide a way to get the dropped frame count when the element is visible for rendering.

Hello Matt,

Not sure who could be the best person to review, would you like to give me some feedbacks for this solution ?

Hi Jya,
What do you think if we just provide dropped frame number which is accumulated when the element is visible for rendering ?
If it's acceptable, I'll then store this information into FrameStatistics.
Flags: needinfo?(jyavenard)
Attachment #8923326 - Flags: feedback?(matt.woodrow)
Comment on attachment 8923326 [details] [diff] [review]
Provide a way to get the dropped frame count when the element is visible for rendering.

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

As I mentioned on irc, the element will be marked as visible when it's currently within the displayport (prerendering area for async scrolling), but not actually being composited.

This will likely cause a lot of frame drops to be reported if you scroll a video just offscreen.

I guess it's more of a media team decision if that's acceptable, but I'd guess that it's not.

I think the only place that we can get a truly correct answer is by checking on the compositor itself.

If we record a frame-id for every composite, then we can detect the case where we have successive composites, but we've skipped over a video frame because it took so long. There's corner cases to deal with, like when we suspended the compositor for some reason, and 'successive composites' are delayed for reasons other than just slowness.

I haven't fully thought it through yet, but something like that seems like it'd give the best results.
(In reply to Kilik Kuo [:kikuo] (PTO:11/23~11/27) from comment #7)
> Hi Jya,
> What do you think if we just provide dropped frame number which is
> accumulated when the element is visible for rendering ?
> If it's acceptable, I'll then store this information into FrameStatistics.

sorry for the late answer..
I'm not sure I understand the question... I would assume that if the element isn't visible, then it's not rendered and as such the number of dropped frames is irrelevant : i.e. should be 0

the number of dropped frames is typically used to determine if we have a problem rendering the current resolution, giving the opportunity to the player to adapt to a lower resolution.
As such, if we only report the number of frames skipped by the compositor on visible item, that should be okay, wanted even.

did I answer your question?
Flags: needinfo?(jyavenard)
We may not need a perfect solution. IIUC, the current solution should be enough and helpful for us to debug some playback issues. We could fire another bug for a better method to handle all the cases in comment 8. 
Matt and Kilik,
What do you think?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(kikuo)
(In reply to Blake Wu [:bwu][:blakewu] from comment #10)
> We may not need a perfect solution. IIUC, the current solution should be
> enough and helpful for us to debug some playback issues. We could fire
> another bug for a better method to handle all the cases in comment 8. 
> Matt and Kilik,
> What do you think?

Sorry for the late response, for me, I think it would do no harm to expose the dropped frame count information in ImageContainer to MediaPanel with explanation which indicates the value is just for reference.
I'll go that way first.
Flags: needinfo?(kikuo)
Depends on: 1429284
Assignee: nobody → jyavenard
We will use the characteristic of which TimedImage is returned to keep track on how many frames were dropped because they were too old. As such, we must make sure the retrieval of the current image is serialised.

This allows to reduce duplicated code between WebRenderImageHost and ImageHost classes.

Additionally, make RenderInfo::img member const as really, we never want to modify that one.

A future change will enforce that RenderInfo.img never survives longer than the ChooseImage()'s caller to clarify the lifetime of the TimedImage.
We can't rely on the FrameID continuity to determine if a frame has been dropped due to timing or not.
The reason being that the VideoSink will not send to the compositor frames it knows as being late already (causing a discontinuity in the frames IDs), and count them as being dropped.
If we were to look at discontinuity on the compositor we would account for those frames twice.

FramesID will also increase non-linearly if a frame isn't painted because it's not visible (either out of the visible tree or in a hidden tab).

What we can measure however, is when a frame should have been painted but didn't because it was too late by looking at the value returned by ImageComposite::ChooseImageIndex() or when a new set of images is being received by the ImageComposite.
Any images found in the earlier array but never returned must have been dropped due to timing.

Looking at the index continuity greatly simplify the logic as we no longer need to worry if a video is hidden or not, or be part of a layer that is itself hidden as neither SetImages will be called then, nor ChooseImage

For now, we only account for those frames dropped, and do not report them yet.

Depends on D2175
We report the number of frames dropped by the compositor because they were too late through:
ImageComposite -> ImageHost -> CompositableTransactionParent -> ImageBridgeParent -> IPDL -> ImageBridgeChild -> ImageContainerListener -> ImageContainer -> VideoSink

Depends on D2176
Also speed up compositing videos as there's no longer need to check every single frames twice to determine if they were composited or not.

Depends on D2177
Attachment #8923326 - Attachment is obsolete: true
Attachment #8923326 - Flags: feedback?(matt.woodrow)
Comment on attachment 8992469 [details]
Bug 1245400 - P2. Keep track of frames that should have been painted but didn't. r?nical, r?mattwoodrow

:nical , can you please create a Phabricator account? thank you
Attachment #8992469 - Flags: review?(nical.bugzilla)
Attachment #8992468 - Flags: review?(nical.bugzilla)
Comment on attachment 8992468 [details]
Bug 1245400 - P1. Make ImageComposite::mImages a private member. r?nical

Nicolas Silva [:nical] has approved the revision.

https://phabricator.services.mozilla.com/D2175
Attachment #8992468 - Flags: review+
Attachment #8992552 - Attachment description: Bug 1245400 - P5. Report frames dropped with WebRender. r?nical Depends on D2178 → Bug 1245400 - P5. Report frames dropped with WebRender. r?nicalDepends on D2178
Comment on attachment 8992469 [details]
Bug 1245400 - P2. Keep track of frames that should have been painted but didn't. r?nical, r?mattwoodrow

Nicolas Silva [:nical] has approved the revision.

https://phabricator.services.mozilla.com/D2176
Attachment #8992469 - Flags: review+
Comment on attachment 8992471 [details]
Bug 1245400 - P4. Remove no longer used class member. r?mattwoodrow

Nicolas Silva [:nical] has approved the revision.

https://phabricator.services.mozilla.com/D2178
Attachment #8992471 - Flags: review+
Comment on attachment 8992552 [details]
Bug 1245400 - P5. Report frames dropped with WebRender. r?nicalDepends on D2178

Nicolas Silva [:nical] has approved the revision.

https://phabricator.services.mozilla.com/D2182
Attachment #8992552 - Flags: review+
Comment on attachment 8992470 [details]
Bug 1245400 - P3. Report number of frames dropped by compositor back to VideoSink. r?mattwoodrow

Nicolas Silva [:nical] has approved the revision.

https://phabricator.services.mozilla.com/D2177
Attachment #8992470 - Flags: review+
Attachment #8992468 - Flags: review?(nical.bugzilla)
Attachment #8992469 - Flags: review?(nical.bugzilla)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/257fe22ca35f
P1. Make ImageComposite::mImages a private member. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0995cbb3df
P2. Keep track of frames that should have been painted but didn't. r=nical, r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ca44a69f9f
P3. Report number of frames dropped by compositor back to VideoSink. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd2152bd331
P4. Remove no longer used class member. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef0a3315910
P5. Report frames dropped with WebRender. r=nical
You need to log in before you can comment on or make changes to this bug.