Send timestamped images from video decoding to the compositor

RESOLVED FIXED in Firefox 42

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla42
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(64 attachments, 1 obsolete attachment)

29.66 KB, text/plain
Details
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
nical
: review+
Details | Review
40 bytes, text/x-review-board-request
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
cpearce
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
nical
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
nical
: review+
sotaro
: review+
Details | Review
40 bytes, text/x-review-board-request
nical
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
Details | Review
40 bytes, text/x-review-board-request
cpearce
: review+
Details | Review
40 bytes, text/x-review-board-request
Details | Review
Comment hidden (empty)
I have this basically working. Still working on getting it to build and pass tests on all platforms.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae65256a240a

On my desktop Linux setup (OMTC, Basic compositor, no hardware vsync) it doesn't provide much benefit on a regular ~25fps video. While my machine is idle, we basically never skip frames with or without these patches. With these patches, once in a while we display the second frame in the queue, so the frame timing is a little more accurate, but that never results in a frame being rendered that would otherwise be completely skipped.

When I load the machine by doing a FF build, we sometimes skip a lot of frames, but that's always because the compositor thread didn't get scheduled for a while. So to get maximum benefit from this we need to have the compositor thread scheduled more reliably, by tweaking thread priorities and/or using HW vsync.

I'm also testing http://media.junglecode.net/test/1077519/test.html. It doesn't get good results yet, apparently because of a bug where the MDSM thread too eagerly removes frames from the head of the queue that we need to display...
Flags: needinfo?(mchang)
Sorry, didn't mean to needinfo.
Flags: needinfo?(mchang)
Created attachment 8604442 [details]
output2

Here's a log showing what happens during a run of Jet's gray-box test. There are several different issues, none of which seem to be related to these patches:

There are some issues in the startup phase:
1) We skip the first frame because by the time the first data reaches the compositor the very first frame is out of date.
2) Right after the first composite, there's a massive compositor delay (2.3s in which the compositor doesn't run). This obviously causes a lot of frames to be skipped. Not sure what causes that.
3) After the massive compositor delay, the media stack throws out a lot of frames and the next frames it produces are significantly ahead of the current time. Maybe this is keyframe skipping?

In the steady state there are two main issues:
4) Sometimes the compositor just doesn't get scheduled. At more or less random intervals, the delay between composites is 33/34ms (i.e. 2 frames at 60fps) instead of 16/17ms. I don't know what's causing this. I suspected kernel scheduling but the fact that it's exactly two frames (exactly 3 frames in just one case) suggests that it might be our bug. This is the most frequently observed problem.
5) The video rate isn't exactly matched to the compositor rate even though they're both trying to hit 60fps. You can see how the delay values (current time - frame time) slowly drift, usually increasing but sometimes decreasing for a while. From frameID 20 to 70 and from frameID 130 to 170 the delay drifts up and then drifts down again. A problem occurs when the delay drifts across 0, i.e. when the frame times almost exactly match the composition time. At that point, very small amounts of jitter can cause a video frame time to land either side of the composition time, giving us two or zero video frames in a composition interval.

At the end:
6) For the last couple of frames, the media stack repeatedly sends them to the compositor with their timestamps increasing every time it sends them. There's some kind of shutdown issue I guess.
Flags: needinfo?(mchang)
In this log, there are three times where we select a frame that's not first in the queue to display the next frame in sequence, and we're not adjacent to one of the above problems. Those three times --- frameID 324, 356-358, and 479 --- are the places where fixing this bug enables us to avoid a glitch. So this work helps, but fixing the above problems, especially #4 and #5, will help more.

Jet's testcase has an audio track, so the drift in issue #5 is likely just the audio clock drifting vs real-time clock (or screen vsync, if we had that enabled). So even though the video and screen rates are both nominally 60fps, those rates can drift relative to one another and it's not clear to me what we should do to avoid problems with jitter when the timestamps are nearly aligned. The best I can think of is a heuristic so that if consecutive frame timestamps are drifting from being slightly before the composition time to slightly after, we apply a negative bias to hold them before the composition time, until they exceed the composition time by some threshold, at which point we remove the bias. (And something similar for the other direction.) That would ensure we only miss or dup one frame instead of several in a row as we see in this log (e.g. in this log, frameIDs 177 and 179 are both skipped and frameIDs 176, 178 and 180 are duped; all we really need to do is dup one frame). Jesup, got any suggestions? :-)
Flags: needinfo?(rjesup)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Jet's testcase has an audio track, so the drift in issue #5 is likely just
> the audio clock drifting vs real-time clock (or screen vsync, if we had that
> enabled). So even though the video and screen rates are both nominally
> 60fps, those rates can drift relative to one another and it's not clear to
> me what we should do to avoid problems with jitter when the timestamps are
> nearly aligned. The best I can think of is a heuristic so that if
> consecutive frame timestamps are drifting from being slightly before the
> composition time to slightly after, we apply a negative bias to hold them
> before the composition time, until they exceed the composition time by some
> threshold, at which point we remove the bias. (And something similar for the
> other direction.) That would ensure we only miss or dup one frame instead of
> several in a row as we see in this log (e.g. in this log, frameIDs 177 and
> 179 are both skipped and frameIDs 176, 178 and 180 are duped; all we really
> need to do is dup one frame). Jesup, got any suggestions? :-)

Right, some form of bias/PLL/averaging to avoid aligned-clock jitter effects is a common/reasonable solution.
It can't guarantee consistent behavior, but in normal situations so long as the smoothing applied can absorb the amount of timing jitter, it will work well.  Even if jitter exceeds the smoothing levels, it will greatly reduce residual jitter across the timing boundary.  You should be running this more-or-less all the time probably.  Effectively it just needs to provide an appropriate hysteresis across the aligned-clocks state.
Flags: needinfo?(rjesup)
#4 actually seems to be caused by a bug where we're not computing an invalid region for an image layer when there's not been a layer transaction, but the image layer wants to display a new image just because of the composition time advancing. I'll fix that, and that plus the bias fix might solve our problems (apart from the start/end issues).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Jet's testcase has an audio track, so the drift in issue #5 is likely just
> the audio clock drifting vs real-time clock (or screen vsync, if we had that
> enabled). So even though the video and screen rates are both nominally
> 60fps, those rates can drift relative to one another and it's not clear to
> me what we should do to avoid problems with jitter when the timestamps are
> nearly aligned. The best I can think of is a heuristic so that if
> consecutive frame timestamps are drifting from being slightly before the
> composition time to slightly after, we apply a negative bias to hold them
> before the composition time, until they exceed the composition time by some
> threshold, at which point we remove the bias. (And something similar for the
> other direction.) That would ensure we only miss or dup one frame instead of
> several in a row as we see in this log (e.g. in this log, frameIDs 177 and
> 179 are both skipped and frameIDs 176, 178 and 180 are duped; all we really
> need to do is dup one frame). Jesup, got any suggestions? :-)

From what I gather from the bug, these problems are on Linux. Some composition timing drift is due to software vsync, but I wonder if these results reproduce on Mac or Windows, which have hardware vsync enabled. Linux has "software" vsync enabled, so we could fix some drift if we implement hardware vsync on linux.

For #5, is there any reason we should try to fix this drift versus making the audio and video clock key off the same timer? Then they would be aligned, but I'm not familiar with how video is implemented. It seems like the audio and video timers should be synced.

Also, I checked out the repo at comment 1 but couldn't find where the log was being produced. That would help diagnose this issue a bit better. Thanks!
Flags: needinfo?(mchang)
(In reply to Mason Chang [:mchang] from comment #8)
> From what I gather from the bug, these problems are on Linux.

Right.

> Some composition timing drift is due to software vsync, but I wonder if these
> results reproduce on Mac or Windows, which have hardware vsync enabled.

I'm sure there will still be drift with HW vsync, since the problem is that the audio clock is an independent clock from the video sync clock, regardless of what's driving the video clock.

> For #5, is there any reason we should try to fix this drift versus making
> the audio and video clock key off the same timer? Then they would be
> aligned, but I'm not familiar with how video is implemented. It seems like
> the audio and video timers should be synced.

We can't do that, we're at the mercy of the audio and video hardware here. E.g. even if a 50fps movie has audio and video data spanning exactly 1 second (in terms of timestamps in the media file), it may take slightly more or less than 1 second to play that audio out due to the audio hardware playing slightly slower or faster than "real time". Likewise we might not be able to play exactly 60 frames to the screen, since the screen vsync will not be exactly 60fps. But we can't really fix or work around either of those issues. I think the bias fix will be pretty easy anyway.

> Also, I checked out the repo at comment 1 but couldn't find where the log
> was being produced. That would help diagnose this issue a bit better. Thanks!

Sorry, that's an mq patch not in the queue. I just wanted you to be aware of what's going on here, I don't need anything changed at the moment. I do think we need to think about how to get the compositor thread to be scheduled reliably on a loaded system.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> We can't do that, we're at the mercy of the audio and video hardware here.
> E.g. even if a 50fps movie has audio and video data spanning exactly 1
> second (in terms of timestamps in the media file), it may take slightly more
> or less than 1 second to play that audio out due to the audio hardware
> playing slightly slower or faster than "real time". Likewise we might not be
> able to play exactly 60 frames to the screen, since the screen vsync will
> not be exactly 60fps. But we can't really fix or work around either of those
> issues. I think the bias fix will be pretty easy anyway.
> 

That's a bummer :/

> > Also, I checked out the repo at comment 1 but couldn't find where the log
> > was being produced. That would help diagnose this issue a bit better. Thanks!
> 
> Sorry, that's an mq patch not in the queue. I just wanted you to be aware of
> what's going on here, I don't need anything changed at the moment. I do
> think we need to think about how to get the compositor thread to be
> scheduled reliably on a loaded system.

Do you know how long the composites are taking? If the composites are short and we can decode the video on hardware, I'm less worried about scheduling composition reliably versus just ensuring the composite finishes before the next vblank. As long as we composite in time, it doesn't matter when we start composting versus we finish in time.
I think I've fixed the invalidation and bias issues. Results have improved on Jet's test but I still see some problems:
-- The starting and ending issues are still there.
-- Sometimes moving the mouse seems to trigger missed compositor deadlines. rr suggests that's because the compositor is blocking on X11 display locks held by the main thread during synchronous X server round trips to convert mouse event coordinates.
-- Sometimes even when my results suggest we didn't miss any frames during composition, I still see missed frames visually. This could be our software vsync timer jittering against screen vsync, or the gnome-shell compositor messing things up.

On the bright side, my patch queue is definitely improving things. It regularly picks a queue entry > 0 to render at times when we would otherwise have displayed a glitch. And we're certainly doing better than Chrome :-).
> Do you know how long the composites are taking? If the composites are short
> and we can decode the video on hardware, I'm less worried about scheduling
> composition reliably versus just ensuring the composite finishes before the
> next vblank. As long as we composite in time, it doesn't matter when we
> start composting versus we finish in time.

Sometimes we're not able to finish the composite before the next vsync signal in which case we're just skipping the composite because CompositorVsyncScheduler::NotifyVsync sees that mCurrentCompositeTask is not null. Maybe we should have some telemetry there?

Looking at the compositor thread when that happens, I've seen it in the compositor waiting on an X11 lock held by the main thread (see above), and I've seen it in a LayerBridge update.
Windows builds fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14cbe5450224
We appear to be green on desktop and B2G. On Android we failed in dom/media/mediasource/test/test_HaveMetadataUnbufferedSeek.html, which I've confirmed is a bug in the test. The final issue is that we're crashing in robocop tests such as testAboutPage. I'm looking into that now.
Created attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Attachment #8622203 - Flags: review?(nical.bugzilla)
Created attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Resizing a WebGL canvas should trigger PresentScreenBuffer. r=jgilbert

At the end of drawingbuffer-static-canvas-test.html, the test resizes the
canvas and does no drawing. The canvas layer changes size but the front
buffer size is unchanged, and this mismatch triggers assertions that later
patches will add. I guess it could also trigger rendering issues.
Attachment #8622204 - Flags: review?(jgilbert)
Created attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Attachment #8622205 - Flags: review?(nical.bugzilla)
Created attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Attachment #8622206 - Flags: review?(bent.mozilla)
Created attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Attachment #8622207 - Flags: review?(cpearce)
Created attachment 8622208 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical

Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical
Attachment #8622208 - Flags: review?(nical.bugzilla)
Created attachment 8622209 [details]
MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce

Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical
Attachment #8622209 - Flags: review?(nical.bugzilla)
Created attachment 8622210 [details]
MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical

Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce
Attachment #8622210 - Flags: review?(cpearce)
Created attachment 8622211 [details]
MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

Bug 1143575. Make GL context current before cleaning up programs. r=nical

Otherwise we can get a crash with the following stack:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 14711]
0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
683	        MOZ_ASSERT(IsCurrent());
(gdb) where
#0  0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
#1  0x5d99bed6 in mozilla::gl::GLContext::raw_fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2232
#2  0x5d99c10a in mozilla::gl::GLContext::fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2270
#3  0x5daa0ae6 in mozilla::layers::ShaderProgramOGL::~ShaderProgramOGL (this=0x6d7df000, __in_chrg=<optimized out>)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/OGLShaderProgram.cpp:491
#4  0x5da86bdc in mozilla::layers::CompositorOGL::CleanupResources (this=0x67ae4d70)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/CompositorOGL.cpp:177
Attachment #8622211 - Flags: review?(nical.bugzilla)
Created attachment 8622212 [details]
MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

There is some ambiguity about whether ScheduleComposite will necessarily
trigger a composite all the way to nsWindow::DrawWindowUnderlay. Android
robocop tests assume it will, because they rely on DrawWindowOverlay
being called so they can take a screenshot and make progress,
but this is a very fragile assumption. They also rely on the entire
window being painted, which is also a fragile assumption.

This patch improves the situation by explicitly invalidating the current
window area when Android Java code needs to trigger a composite. This avoids
regressions from future patches in this series which make composition bail
out when there is nothing invalid.

The resulting setup is still a bit fragile for my taste but I'm not sure
what the ideal solution would be.
Attachment #8622212 - Flags: review?(nical.bugzilla)
Created attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical
Attachment #8622213 - Flags: review?(nical.bugzilla)
Created attachment 8622214 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Bug 1143575. Remove unused CompositionNotifySink. r=nical
Attachment #8622214 - Flags: review?(nical.bugzilla)
Created attachment 8622215 [details]
MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical
Attachment #8622215 - Flags: review?(nical.bugzilla)
Created attachment 8622216 [details]
MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical
Attachment #8622216 - Flags: review?(nical.bugzilla)
Created attachment 8622217 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical
Attachment #8622217 - Flags: review?(nical.bugzilla)
Created attachment 8622218 [details]
MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical
Attachment #8622218 - Flags: review?(nical.bugzilla)
Created attachment 8622219 [details]
MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

Bug 1143575. Remove unused ReturnReleaseFence. r=nical
Attachment #8622219 - Flags: review?(nical.bugzilla)
Created attachment 8622220 [details]
MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow
Attachment #8622220 - Flags: review?(matt.woodrow)
Created attachment 8622221 [details]
MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical
Attachment #8622221 - Flags: review?(nical.bugzilla)
Created attachment 8622222 [details]
MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical
Attachment #8622222 - Flags: review?(nical.bugzilla)
Created attachment 8622223 [details]
MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical

Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

This simplifies code slightly.
Attachment #8622223 - Flags: review?(nical.bugzilla)
Created attachment 8622224 [details]
MozReview Request: Bug 1143575. Fix indent. r=cpearce

Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical
Attachment #8622224 - Flags: review?(nical.bugzilla)
Created attachment 8622225 [details]
MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

Bug 1143575. Fix indent. r=cpearce
Attachment #8622225 - Flags: review?(cpearce)
Created attachment 8622226 [details]
MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce
Attachment #8622226 - Flags: review?(cpearce)
Created attachment 8622227 [details]
MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce
Attachment #8622227 - Flags: review?(cpearce)
Created attachment 8622228 [details]
MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce

Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

This makes MediaDecoderStateMachine::GetVideoStreamPosition compute a
time that's more consistent with the audio clock.
Attachment #8622228 - Flags: review?(cpearce)
Created attachment 8622229 [details]
MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce

Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce
Attachment #8622229 - Flags: review?(cpearce)
Created attachment 8622230 [details]
MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

Bug 1143575. Rename clock_time to clockTime. r=cpearce
Attachment #8622230 - Flags: review?(cpearce)
Created attachment 8622231 [details]
MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce

Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

This makes normal playback consistent with the buffering state, which already
does this. We'll also need this when we handle multiple images, because then
we need to hande the entire queue of images to the ImageContainer without
pulling any of them off the queue.
Attachment #8622231 - Flags: review?(cpearce)
Created attachment 8622232 [details]
MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce
Attachment #8622232 - Flags: review?(cpearce)
Created attachment 8622233 [details]
MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical

Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

The picture rect logically belongs with the texture, and later patches will
make OpUseTexture take multiple textures, each of which needs its own
picture rect.
Attachment #8622233 - Flags: review?(nical.bugzilla)
Created attachment 8622234 [details]
MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical

Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical
Attachment #8622234 - Flags: review?(nical.bugzilla)
Created attachment 8622235 [details]
MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical

Bug 1143575. Fix typo in ImageContainer comment. r=nical
Attachment #8622235 - Flags: review?(nical.bugzilla)
Created attachment 8622236 [details]
MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical

Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical
Attachment #8622236 - Flags: review?(nical.bugzilla)
Created attachment 8622237 [details]
MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical

Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical
Attachment #8622237 - Flags: review?(nical.bugzilla)
Created attachment 8622238 [details]
MozReview Request: Bug 1143575. Implement ImageHost support for multiple timed images. r=nical

Bug 1143575. Store composition time in Compositor. r=nical

We'll need this later so ImageHost can select the correct image to use.
Attachment #8622238 - Flags: review?(nical.bugzilla)
Created attachment 8622239 [details]
MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical

Bug 1143575. Implement ImageHost support for multiple timed images. r=nical
Attachment #8622239 - Flags: review?(nical.bugzilla)
Created attachment 8622240 [details]
MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical
Attachment #8622240 - Flags: review?(nical.bugzilla)
Created attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

When ImageContainer and ImageClient are managing a list of images, the
individual Image serial numbers are no longer enough to detect whether the
state has changed.
Attachment #8622241 - Flags: review?(nical.bugzilla)
Created attachment 8622242 [details]
MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical

Bug 1143575. Remove ImageClientBridge::Updated. r=nical
Attachment #8622242 - Flags: review?(nical.bugzilla)
Created attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical
Attachment #8622243 - Flags: review?(nical.bugzilla)
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Created attachment 8622244 [details]
MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro

Bug 1143575. Fix some code formatting. r=nical
Attachment #8622244 - Flags: review?(nical.bugzilla)
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Resizing a WebGL canvas should trigger PresentScreenBuffer. r=jgilbert

At the end of drawingbuffer-static-canvas-test.html, the test resizes the
canvas and does no drawing. The canvas layer changes size but the front
buffer size is unchanged, and this mismatch triggers assertions that later
patches will add. I guess it could also trigger rendering issues.
Created attachment 8622245 [details]
MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical

Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical
Attachment #8622245 - Flags: review?(nical.bugzilla)
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Created attachment 8622246 [details]
MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical
Attachment #8622246 - Flags: review?(nical.bugzilla)
Comment on attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Comment on attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Attachment #8622247 - Flags: review?(nical.bugzilla)
Created attachment 8622247 [details]
MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow

Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

For frame statistics to work properly, we have to notify an ImageContainer
when it has been composited. This requires a few changes, which have
been lumped together in this patch:
-- Create PImageContainer and ImageContainerParent/ImageContainerChild.
-- Add mFrameID and mProducerID everywhere we're passing around images.
-- Route composition notifications from the compositor back to
ImageContainerChild.
Created attachment 8622248 [details]
MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow
Attachment #8622248 - Flags: review?(matt.woodrow)
Comment on attachment 8622208 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical

Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical
Created attachment 8622249 [details]
MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

We need this change so that when ImageHost has a next image to display
more than one composition-interval in the future, we skip the actual
compositing work in those intermediate composition(s) if nothing else
has changed.

This change is a little bit scary since it breaks any code that was
previously assuming ScheduleComposition would actually update the screen.
However, that code was already broken for BasicCompositor.
Attachment #8622249 - Flags: review?(matt.woodrow)
Comment on attachment 8622209 [details]
MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce

Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical
Created attachment 8622250 [details]
MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical

Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

We need to remove this so that adding images to the end of the list of images
for an ImageLayer doesn't force composition to happen even if nothing else
has changed.
Attachment #8622250 - Flags: review?(matt.woodrow)
Created attachment 8622251 [details]
MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce

Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical
Attachment #8622251 - Flags: review?(nical.bugzilla)
Created attachment 8622252 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Bug 1143575. Don't report negative frame delays. r=cpearce
Attachment #8622252 - Flags: review?(nical.bugzilla)
Created attachment 8622253 [details]
MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical
Attachment #8622253 - Flags: review?(nical.bugzilla)
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Resizing a WebGL canvas should trigger PresentScreenBuffer. r=jgilbert

At the end of drawingbuffer-static-canvas-test.html, the test resizes the
canvas and does no drawing. The canvas layer changes size but the front
buffer size is unchanged, and this mismatch triggers assertions that later
patches will add. I guess it could also trigger rendering issues.
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Comment on attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Comment on attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Comment on attachment 8622208 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical

Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical
Comment on attachment 8622209 [details]
MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce

Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical
Comment on attachment 8622210 [details]
MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical

Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce
Comment on attachment 8622211 [details]
MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

Bug 1143575. Make GL context current before cleaning up programs. r=nical

Otherwise we can get a crash with the following stack:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 14711]
0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
683	        MOZ_ASSERT(IsCurrent());
(gdb) where
#0  0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
#1  0x5d99bed6 in mozilla::gl::GLContext::raw_fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2232
#2  0x5d99c10a in mozilla::gl::GLContext::fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2270
#3  0x5daa0ae6 in mozilla::layers::ShaderProgramOGL::~ShaderProgramOGL (this=0x6d7df000, __in_chrg=<optimized out>)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/OGLShaderProgram.cpp:491
#4  0x5da86bdc in mozilla::layers::CompositorOGL::CleanupResources (this=0x67ae4d70)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/CompositorOGL.cpp:177
Comment on attachment 8622212 [details]
MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

There is some ambiguity about whether ScheduleComposite will necessarily
trigger a composite all the way to nsWindow::DrawWindowUnderlay. Android
robocop tests assume it will, because they rely on DrawWindowOverlay
being called so they can take a screenshot and make progress,
but this is a very fragile assumption. They also rely on the entire
window being painted, which is also a fragile assumption.

This patch improves the situation by explicitly invalidating the current
window area when Android Java code needs to trigger a composite. This avoids
regressions from future patches in this series which make composition bail
out when there is nothing invalid.

The resulting setup is still a bit fragile for my taste but I'm not sure
what the ideal solution would be.
Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical
Comment on attachment 8622214 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Bug 1143575. Remove unused CompositionNotifySink. r=nical
Comment on attachment 8622215 [details]
MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical
Comment on attachment 8622216 [details]
MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical
Comment on attachment 8622217 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical
Comment on attachment 8622218 [details]
MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical
Comment on attachment 8622219 [details]
MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

Bug 1143575. Remove unused ReturnReleaseFence. r=nical
Comment on attachment 8622220 [details]
MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow
Comment on attachment 8622221 [details]
MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical
Comment on attachment 8622222 [details]
MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical
Comment on attachment 8622223 [details]
MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical

Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

This simplifies code slightly.
Comment on attachment 8622224 [details]
MozReview Request: Bug 1143575. Fix indent. r=cpearce

Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical
Comment on attachment 8622225 [details]
MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

Bug 1143575. Fix indent. r=cpearce
Comment on attachment 8622226 [details]
MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce
Comment on attachment 8622227 [details]
MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce
Comment on attachment 8622228 [details]
MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce

Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

This makes MediaDecoderStateMachine::GetVideoStreamPosition compute a
time that's more consistent with the audio clock.
Comment on attachment 8622229 [details]
MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce

Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce
Comment on attachment 8622230 [details]
MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

Bug 1143575. Rename clock_time to clockTime. r=cpearce
Comment on attachment 8622231 [details]
MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce

Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

This makes normal playback consistent with the buffering state, which already
does this. We'll also need this when we handle multiple images, because then
we need to hande the entire queue of images to the ImageContainer without
pulling any of them off the queue.
Comment on attachment 8622232 [details]
MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce
Comment on attachment 8622233 [details]
MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical

Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

The picture rect logically belongs with the texture, and later patches will
make OpUseTexture take multiple textures, each of which needs its own
picture rect.
Comment on attachment 8622234 [details]
MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical

Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical
Comment on attachment 8622235 [details]
MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical

Bug 1143575. Fix typo in ImageContainer comment. r=nical
Comment on attachment 8622236 [details]
MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical

Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical
Comment on attachment 8622237 [details]
MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical

Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical
Comment on attachment 8622238 [details]
MozReview Request: Bug 1143575. Implement ImageHost support for multiple timed images. r=nical

Bug 1143575. Store composition time in Compositor. r=nical

We'll need this later so ImageHost can select the correct image to use.
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Comment on attachment 8622239 [details]
MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical

Bug 1143575. Implement ImageHost support for multiple timed images. r=nical
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Resizing a WebGL canvas should trigger PresentScreenBuffer. r=jgilbert

At the end of drawingbuffer-static-canvas-test.html, the test resizes the
canvas and does no drawing. The canvas layer changes size but the front
buffer size is unchanged, and this mismatch triggers assertions that later
patches will add. I guess it could also trigger rendering issues.
Comment on attachment 8622240 [details]
MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Comment on attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

When ImageContainer and ImageClient are managing a list of images, the
individual Image serial numbers are no longer enough to detect whether the
state has changed.
Comment on attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Comment on attachment 8622242 [details]
MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical

Bug 1143575. Remove ImageClientBridge::Updated. r=nical
Comment on attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Comment on attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical
Comment on attachment 8622244 [details]
MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro

Bug 1143575. Fix some code formatting. r=nical
Comment on attachment 8622245 [details]
MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical

Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical
Comment on attachment 8622246 [details]
MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical
Comment on attachment 8622247 [details]
MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow

Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

For frame statistics to work properly, we have to notify an ImageContainer
when it has been composited. This requires a few changes, which have
been lumped together in this patch:
-- Create PImageContainer and ImageContainerParent/ImageContainerChild.
-- Add mFrameID and mProducerID everywhere we're passing around images.
-- Route composition notifications from the compositor back to
ImageContainerChild.
Comment on attachment 8622248 [details]
MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow
Comment on attachment 8622249 [details]
MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

We need this change so that when ImageHost has a next image to display
more than one composition-interval in the future, we skip the actual
compositing work in those intermediate composition(s) if nothing else
has changed.

This change is a little bit scary since it breaks any code that was
previously assuming ScheduleComposition would actually update the screen.
However, that code was already broken for BasicCompositor.
Comment on attachment 8622250 [details]
MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical

Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

We need to remove this so that adding images to the end of the list of images
for an ImageLayer doesn't force composition to happen even if nothing else
has changed.
Comment on attachment 8622251 [details]
MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce

Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical
Comment on attachment 8622252 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Bug 1143575. Don't report negative frame delays. r=cpearce
Comment on attachment 8622253 [details]
MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical
Created attachment 8622255 [details]
MozReview Request: Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical

Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

ImageBridgeChild::FlushAllImages with aExceptFront==true does absolutely
nothing, so remove the parameter and remove all callers which pass true.
Attachment #8622255 - Flags: review?(nical.bugzilla)
Created attachment 8622256 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical

We need to make it clear that ClearCurrentImage is really an internal method
of the ImageContainer implementation, not a method that ImageContainer users
should call.
Attachment #8622256 - Flags: review?(nical.bugzilla)
Created attachment 8622257 [details]
MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical

Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical
Attachment #8622257 - Flags: review?(nical.bugzilla)
Created attachment 8622258 [details]
MozReview Request: Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical

Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical
Attachment #8622258 - Flags: review?(nical.bugzilla)
Created attachment 8622259 [details]
MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical

Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical
Attachment #8622259 - Flags: review?(nical.bugzilla)
Created attachment 8622260 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce

Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical
Attachment #8622260 - Flags: review?(nical.bugzilla)
Created attachment 8622261 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce
Attachment #8622261 - Flags: review?(cpearce)
Created attachment 8622262 [details]
MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce
Attachment #8622262 - Flags: review?(cpearce)
Created attachment 8622263 [details]
MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce

Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce
Attachment #8622263 - Flags: review?(cpearce)
Created attachment 8622264 [details]
MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce

Bug 1143575. Add frame IDs to VideoData. r=cpearce
Attachment #8622264 - Flags: review?(cpearce)
Created attachment 8622265 [details]
MozReview Request: Bug 1143575. Push all available frames to the compositor. r=cpearce

Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce
Attachment #8622265 - Flags: review?(cpearce)
Created attachment 8622266 [details]
MozReview Request: Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical

Bug 1143575. Push all available frames to the compositor. r=cpearce
Attachment #8622266 - Flags: review?(cpearce)
Created attachment 8622267 [details]
MozReview Request: Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical

Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical
Attachment #8622267 - Flags: review?(nical.bugzilla)
The patches in the "Mozreview request" list in this bug appear out of order. The patches in the attachments list are in the correct order.

Note that the first 30 patches (up to and including "Remove unused MediaQueue::Empty") are refactorings and bug fixes that should make sense independent of the actual work in this bug. The remaining 34 patches do the work of this bug. They're all reasonably small and self-contained except for "Route ImageCompositeNotifications to ImageContainers".
https://reviewboard.mozilla.org/r/11269/#review9713

::: dom/media/MediaDecoderStateMachine.cpp:2991
(Diff revision 1)
> -  int64_t delay = remainingTime / mPlaybackRate;
> +    int64_t delay = remainingTime / mPlaybackRate;

tab

::: dom/media/MediaDecoderStateMachine.cpp:2993
(Diff revision 1)
>      ScheduleStateMachineIn(delay);

MOZ_ASSERT(aMicroseconds > 0) at 3252.
So delay can't be 0.
https://reviewboard.mozilla.org/r/11333/#review9717

::: dom/media/MediaData.h:274
(Diff revision 1)
> +  int32_t mFrameID;

Could it be a const?
https://reviewboard.mozilla.org/r/11333/#review9721

::: dom/media/MediaData.h:274
(Diff revision 1)
> +  int32_t mFrameID;

No. It is updated in MediaDecoderStateMachine::Push in next patch.
https://reviewboard.mozilla.org/r/11269/#review9723

> tab

Oops. this is not a tab but a mark from review board to indicate indent changes.
https://reviewboard.mozilla.org/r/11335/#review9719

::: dom/media/MediaDecoderStateMachine.h:526
(Diff revision 1)
> +  // timestamps for the frames; when omitted, aMaxFrames must be 1 and the

s/and the/and/

::: dom/media/MediaDecoderStateMachine.h:753
(Diff revision 1)
> +  ProducerID mProducerID;

const

::: dom/media/MediaDecoderStateMachine.cpp:901
(Diff revision 1)
> +  aSample->mFrameID = ++mCurrentFrameID;

Is it ok to update the frame ID again when the popped video sample is pushed back to the queue in UpdateRenderedVideoFrames().

::: dom/media/MediaDecoderStateMachine.cpp:1267
(Diff revision 1)
> +    RenderVideoFrames(1);

Why needing to render video frames here?

::: dom/media/MediaDecoderStateMachine.cpp:2972
(Diff revision 1)
> +      frameStats.NotifyPresentedFrame();

Shouldn't we call NotifyPresentedFrame() as many times as framesRemoved?

::: dom/media/MediaDecoderStateMachine.h:523
(Diff revision 1)
> +  // state machine thread. Decode monitor must be held with exactly one lock

It would be nice to add some checks to ensure no re-entry into this function so lock count == 1 can hold.
https://reviewboard.mozilla.org/r/11269/#review9727

> MOZ_ASSERT(aMicroseconds > 0) at 3252.
> So delay can't be 0.

True.

I'll make 'delay' a minimum of 1, then. The only way it could reach 0 is if remainingTime was very small and then got rounded down to zero when dividing by mPlaybackRate.
https://reviewboard.mozilla.org/r/11335/#review9729

> s/and the/and/

Will fix.

> It would be nice to add some checks to ensure no re-entry into this function so lock count == 1 can hold.

OK, I'll do that.

> const

OK

> Is it ok to update the frame ID again when the popped video sample is pushed back to the queue in UpdateRenderedVideoFrames().

The code in UpdateRenderedVideoFrames() calls VideoQueue().PushFront() which doesn't reach this line.

> Why needing to render video frames here?

We have to call RenderVideoFrames(1) in StopPlayback() because we may have queued multiple frames in the compositor. If we don't call RenderVideoFrames(1), those frames will stay queued in the compositor so video playback will continue until the compositor reaches the end of the queue. I'll put this in a comment.

> Shouldn't we call NotifyPresentedFrame() as many times as framesRemoved?

Maybe, but the code I'm changing here doesn't call NotifyPresentedFrame() for frames that were skipped and never sent to the compositor, so I preserved that behavior. Let's see what cpearce says.
Comment on attachment 8622220 [details]
MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

https://reviewboard.mozilla.org/r/11247/#review9737

Ship It!
Attachment #8622220 - Flags: review?(matt.woodrow) → review+
Attachment #8622248 - Flags: review?(matt.woodrow)
Comment on attachment 8622248 [details]
MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

https://reviewboard.mozilla.org/r/11303/#review9739

::: gfx/layers/LayerTreeInvalidation.cpp:411
(Diff revision 1)
> +        if (host) {

else if?
Comment on attachment 8622248 [details]
MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

https://reviewboard.mozilla.org/r/11303/#review9741

Ship It!
Attachment #8622248 - Flags: review+
Attachment #8622249 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8622249 [details]
MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

https://reviewboard.mozilla.org/r/11305/#review9743

Ship It!
Attachment #8622250 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8622250 [details]
MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical

https://reviewboard.mozilla.org/r/11307/#review9745

Ship It!
Wow that's a lot of reviews and I am on PTO until Whistler, I'll start doing a few reviews this week but I expect to get most of them done in Whistler, is that OK?
Sure. I understand it's a lot of reviews :-)
https://reviewboard.mozilla.org/r/11335/#review9821

::: dom/media/MediaDecoderStateMachine.cpp:901
(Diff revision 1)
> +  aSample->mFrameID = ++mCurrentFrameID;

Got it.

::: dom/media/MediaDecoderStateMachine.cpp:1267
(Diff revision 1)
> +    RenderVideoFrames(1);

I still can't understand how RenderVideoFrames(1) will flush frames in the compositor. Is there any way to tell the compositor to discard frames that are queued but not rendered yet?

::: dom/media/MediaDecoderStateMachine.cpp:2972
(Diff revision 1)
> +      frameStats.NotifyPresentedFrame();

I see. This makes sense.
Comment on attachment 8622225 [details]
MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

https://reviewboard.mozilla.org/r/11257/#review9913

Ship It!
Attachment #8622225 - Flags: review?(cpearce) → review+
Comment on attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

https://reviewboard.mozilla.org/r/11221/#review9917

Ship It!
Attachment #8622207 - Flags: review?(cpearce) → review+
Attachment #8622210 - Flags: review?(cpearce) → review+
Comment on attachment 8622210 [details]
MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical

https://reviewboard.mozilla.org/r/11227/#review9919

Ship It!
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

https://reviewboard.mozilla.org/r/11215/#review9925

::: dom/canvas/WebGLContextGL.cpp:144
(Diff revision 1)
> +    ClearBackbufferIfNeeded();

This should not be here.

::: dom/canvas/WebGLContext.cpp:862
(Diff revision 1)
> +        mShouldPresent = true;

I do not think we want to mark this for presentation in the absence of drawing.

This will frequently cause black flashing after resize.

::: dom/canvas/WebGLContext.cpp
(Diff revision 1)
> -        PresentScreenBuffer();

This will cause us to discard anything drawn between two resize events if there is no normal Present called between them. This could cause odd behavior when the user is drag-resizing. Lots of rendered-but-not-displayed frames.
Attachment #8622204 - Flags: review?(jgilbert)
Alright, tell me how to fix the bug properly.
Flags: needinfo?(jgilbert)
Comment on attachment 8622231 [details]
MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce

https://reviewboard.mozilla.org/r/11269/#review9923

::: dom/media/MediaData.h:76
(Diff revision 1)
> +  bool mSentToCompositor;

Why not put this on VideoData? It doesn't make sense for AudioData and MediaRawData, right?

::: dom/media/MediaDecoderStateMachine.cpp:2990
(Diff revision 1)
> -  // The remainingTime is negative (include zero):
> +  if (remainingTime < INT64_MAX) {

remainingTime is an int64_t, so how could this condition ever be false?
Attachment #8622231 - Flags: review?(cpearce)
https://reviewboard.mozilla.org/r/11335/#review9929

> I still can't understand how RenderVideoFrames(1) will flush frames in the compositor. Is there any way to tell the compositor to discard frames that are queued but not rendered yet?

Yes. Every time RenderVideoFrames is called, it completely replaces the set of frames queued at the compositor. So when we call it here, we replace whatever frames are queued with a single frame.
https://reviewboard.mozilla.org/r/11269/#review9931

> Why not put this on VideoData? It doesn't make sense for AudioData and MediaRawData, right?

Sure, I'll move it to VideoData.

> remainingTime is an int64_t, so how could this condition ever be false?

You're right. I'll remove this check.
Attachment #8622264 - Flags: review?(cpearce)
Comment on attachment 8622264 [details]
MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce

https://reviewboard.mozilla.org/r/11333/#review9933

::: dom/media/MediaData.cpp:258
(Diff revision 1)
> +                                        0));

Why it OK to pass 0 as a frame ID here and elsewhere? In what cases is it needed and not needed?
https://reviewboard.mozilla.org/r/11333/#review9935

> Why it OK to pass 0 as a frame ID here and elsewhere? In what cases is it needed and not needed?

We fill the frame ID in later, in MediaDecoderStateMachine::Push.
Attachment #8622265 - Flags: review?(cpearce)
Comment on attachment 8622265 [details]
MozReview Request: Bug 1143575. Push all available frames to the compositor. r=cpearce

https://reviewboard.mozilla.org/r/11335/#review10031

::: dom/media/MediaDecoderStateMachine.cpp:2972
(Diff revision 1)
> +      frameStats.NotifyPresentedFrame();

"Presented" means "sent to the compositor", so not calling it for skipped frames is expected.

It seems to me however, that the call to NotifyPresentedFrame() makes most sense to be made inside RenderVideoFrame() (as that's the thing that sends frames to the compositor), whenever we encounter a frame with a !mSentToCompositor field. Otherwise, we'll be calling NotifyPresentedFrame() whenever we call RenderVideoFrame(), right?
Attachment #8622226 - Flags: review?(cpearce) → review+
Comment on attachment 8622226 [details]
MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

https://reviewboard.mozilla.org/r/11259/#review10033
Attachment #8622227 - Flags: review?(cpearce) → review+
Comment on attachment 8622227 [details]
MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

https://reviewboard.mozilla.org/r/11261/#review10035

Ship It!
Comment on attachment 8622228 [details]
MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce

https://reviewboard.mozilla.org/r/11263/#review10037

Ship It!
Attachment #8622228 - Flags: review?(cpearce) → review+
Comment on attachment 8622229 [details]
MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce

https://reviewboard.mozilla.org/r/11265/#review10039

::: dom/media/MediaDecoderStateMachine.cpp:3325
(Diff revision 1)
>    mPlaybackRate = mLogicalPlaybackRate;

It's a shame we're not using state mirroring for the playback rate.
Attachment #8622229 - Flags: review?(cpearce)
Comment on attachment 8622229 [details]
MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce

https://reviewboard.mozilla.org/r/11265/#review10041

Ship It!
Attachment #8622229 - Flags: review+
Comment on attachment 8622230 [details]
MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

https://reviewboard.mozilla.org/r/11267/#review10043

Ship It!
Attachment #8622230 - Flags: review?(cpearce) → review+
Comment on attachment 8622232 [details]
MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

https://reviewboard.mozilla.org/r/11271/#review10045

Ship It!
Attachment #8622232 - Flags: review?(cpearce) → review+
Comment on attachment 8622261 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

https://reviewboard.mozilla.org/r/11327/#review10047

Ship It!
Attachment #8622261 - Flags: review?(cpearce) → review+
Attachment #8622262 - Flags: review?(cpearce) → review+
Comment on attachment 8622262 [details]
MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

https://reviewboard.mozilla.org/r/11329/#review10049

Ship It!
Comment on attachment 8622263 [details]
MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce

https://reviewboard.mozilla.org/r/11331/#review10051

Ship It!
Attachment #8622263 - Flags: review?(cpearce) → review+
Comment on attachment 8622264 [details]
MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce

https://reviewboard.mozilla.org/r/11333/#review10055

Ship It!
Attachment #8622264 - Flags: review+
Attachment #8622266 - Flags: review?(cpearce) → review+
Comment on attachment 8622266 [details]
MozReview Request: Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical

https://reviewboard.mozilla.org/r/11337/#review10057

Ship It!
Comment on attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

https://reviewboard.mozilla.org/r/11219/#review10277

Ship It!
Attachment #8622206 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

https://reviewboard.mozilla.org/r/11213/#review10289

Ship It!
Attachment #8622203 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

https://reviewboard.mozilla.org/r/11217/#review10291

Ship It!
Attachment #8622205 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622208 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical

https://reviewboard.mozilla.org/r/11223/#review10299

Ship It!
Attachment #8622208 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622209 [details]
MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce

https://reviewboard.mozilla.org/r/11225/#review10301

Ship It!
Attachment #8622209 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622211 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622211 [details]
MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

https://reviewboard.mozilla.org/r/11229/#review10303

Ship It!
Comment on attachment 8622212 [details]
MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

https://reviewboard.mozilla.org/r/11231/#review10305

Ship It!
Attachment #8622212 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622213 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

https://reviewboard.mozilla.org/r/11233/#review10307

Ship It!
Comment on attachment 8622214 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

https://reviewboard.mozilla.org/r/11235/#review10309

Ship It!
Attachment #8622214 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622215 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622215 [details]
MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

https://reviewboard.mozilla.org/r/11237/#review10311

Ship It!
Comment on attachment 8622216 [details]
MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

https://reviewboard.mozilla.org/r/11239/#review10329

Ship It!
Attachment #8622216 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622217 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

https://reviewboard.mozilla.org/r/11241/#review10331

Ship It!
Attachment #8622217 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622218 [details]
MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical

https://reviewboard.mozilla.org/r/11243/#review10333

Ship It!
Attachment #8622218 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622219 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622219 [details]
MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

https://reviewboard.mozilla.org/r/11245/#review10335

Ship It!
Comment on attachment 8622221 [details]
MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

https://reviewboard.mozilla.org/r/11249/#review10337

Ship It!
Attachment #8622221 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622222 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622222 [details]
MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

https://reviewboard.mozilla.org/r/11251/#review10395

Ship It!
Comment on attachment 8622223 [details]
MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical

https://reviewboard.mozilla.org/r/11253/#review10397

Ship It!
Attachment #8622223 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622224 [details]
MozReview Request: Bug 1143575. Fix indent. r=cpearce

https://reviewboard.mozilla.org/r/11255/#review10399

Ship It!
Attachment #8622224 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622233 [details]
MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical

https://reviewboard.mozilla.org/r/11273/#review10401

Ship It!
Attachment #8622233 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622234 [details]
MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical

https://reviewboard.mozilla.org/r/11275/#review10403

Ship It!
Attachment #8622234 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622235 [details]
MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical

https://reviewboard.mozilla.org/r/11277/#review10405

Ship It!
Attachment #8622235 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622236 [details]
MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical

https://reviewboard.mozilla.org/r/11279/#review10407

Ship It!
Attachment #8622236 - Flags: review?(nical.bugzilla) → review+
Roc, Why are we setting the composition time on the compositor just before scheduling the composition rather than just before doing the actual composition?

It's not extremely important but I'd rather have an api like Compositor::BeginFrame(..., TimeStamp aCompositionTime) than Compositor::SetCompositionTime(TimeStamp aCompositionTime).
Comment on attachment 8622244 [details]
MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro

https://reviewboard.mozilla.org/r/11295/#review10493

Ship It!
Attachment #8622244 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

https://reviewboard.mozilla.org/r/11293/#review10495

Ship It!
Attachment #8622243 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622241 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

https://reviewboard.mozilla.org/r/11289/#review10501

Ship It!
Comment on attachment 8622242 [details]
MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical

https://reviewboard.mozilla.org/r/11291/#review10503

Ship It!
Attachment #8622242 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622246 [details]
MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

https://reviewboard.mozilla.org/r/11299/#review10515

Ship It!
Attachment #8622246 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622251 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622251 [details]
MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce

https://reviewboard.mozilla.org/r/11309/#review10523

Ship It!
Comment on attachment 8622252 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

https://reviewboard.mozilla.org/r/11311/#review10525

Ship It!
Attachment #8622252 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622253 [details]
MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

https://reviewboard.mozilla.org/r/11313/#review10533

Ship It!

::: gfx/layers/ImageContainer.cpp:19
(Diff revision 1)
> +#include "mozilla/layers/LayersMessages.h"  // for ImageClient

nit: remove the "// for ImageClient", looks like a copy-pasting mistake
Attachment #8622253 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8622255 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622255 [details]
MozReview Request: Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical

https://reviewboard.mozilla.org/r/11315/#review10543

Ship It!
(In reply to Nicolas Silva [:nical] from comment #213)
> Roc, Why are we setting the composition time on the compositor just before
> scheduling the composition rather than just before doing the actual
> composition?

We set it in CompositorParent::CompositeToTarget. That's not "just before scheduling the composition" ... right?
Flags: needinfo?(nical.bugzilla)
https://reviewboard.mozilla.org/r/11335/#review10031

> "Presented" means "sent to the compositor", so not calling it for skipped frames is expected.
> 
> It seems to me however, that the call to NotifyPresentedFrame() makes most sense to be made inside RenderVideoFrame() (as that's the thing that sends frames to the compositor), whenever we encounter a frame with a !mSentToCompositor field. Otherwise, we'll be calling NotifyPresentedFrame() whenever we call RenderVideoFrame(), right?

We can do it that way, but that would mean for example that if we decode 10 frames, send them all to the compositor, but stop playback after the first frame has been composited but none of the others, then we'll record having presented 10 frames. Is that OK?
https://reviewboard.mozilla.org/r/11215/#review9925

> I do not think we want to mark this for presentation in the absence of drawing.
> 
> This will frequently cause black flashing after resize.

After further discussion I agree we should drop this patch and I'll adjust the picture-rect asserts to ignore canvas.
Flags: needinfo?(jgilbert)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #223)
> (In reply to Nicolas Silva [:nical] from comment #213)
> > Roc, Why are we setting the composition time on the compositor just before
> > scheduling the composition rather than just before doing the actual
> > composition?
> 
> We set it in CompositorParent::CompositeToTarget. That's not "just before
> scheduling the composition" ... right?

Indeed, my bad. Although, I still think that we should incorporate the timestamp in BeginFrame rather than in a separate method (which is not used near the code that actually calls BeginFrame and all). I am not too found of the idea that as the code evolves and moves around, there is a risk that we might start compositing without having set the proper timestamp (and end up with the previous timestamp which is not quite right but sufficiently subtle that it may take more time for us to figure out why frames seem to drop in rare cases, etc).

Would that work for you?
Flags: needinfo?(nical.bugzilla) → needinfo?(roc)
Sure, I'll do that.
Flags: needinfo?(roc)
Comment on attachment 8622240 [details]
MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

https://reviewboard.mozilla.org/r/11287/#review10841

Ship It!
Attachment #8622240 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622256 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

https://reviewboard.mozilla.org/r/11317/#review10843

Ship It!
Attachment #8622256 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622257 [details]
MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical

https://reviewboard.mozilla.org/r/11319/#review10845

Ship It!
Attachment #8622257 - Flags: review?(nical.bugzilla) → review+
Sotaro, could you have a look at this patch https://reviewboard.mozilla.org/r/11297/diff/1#index_header ?
I can't add reviewers from reviewboard (I think that only the patch subimtter can at the moment) but this part should have your approval.
Flags: needinfo?(sotaro.ikeda.g)

Updated

3 years ago
Attachment #8622247 - Flags: review?(nical.bugzilla)
Comment on attachment 8622247 [details]
MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow

https://reviewboard.mozilla.org/r/11301/#review10847

::: gfx/layers/composite/LayerManagerComposite.h:21
(Diff revision 1)
> +#include "mozilla/layers/LayersMessages.h"  // for LayersBackend, etc

nit: remove the comment on this line, looks like it's a copy-paste leftover from the line below.

::: gfx/layers/ipc/ShadowLayerParent.cpp:43
(Diff revision 1)
> +  Disconnect();

In what situation can Bind be called and the ShadowLayerParent is already be connected to something?
Currently we always call Bind on a layer that we just created but this change suggests otherwise. If this situation is actually possible, wouldn't it make sense to only disconnect if layer != mLayer ?

Updated

3 years ago
Attachment #8622258 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622258 [details]
MozReview Request: Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical

https://reviewboard.mozilla.org/r/11321/#review10851

Ship It!

Updated

3 years ago
Attachment #8622259 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622259 [details]
MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical

https://reviewboard.mozilla.org/r/11323/#review10853

::: dom/media/VideoFrameContainer.h:81
(Diff revision 1)
> +  ImageContainer::FrameID mFrameID;

nit: it'd be nice to have a comment here explaining why we keep track of a frame id.

Updated

3 years ago
Attachment #8622260 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622260 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce

https://reviewboard.mozilla.org/r/11325/#review10855

Ship It!

Updated

3 years ago
Attachment #8622237 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622237 [details]
MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical

https://reviewboard.mozilla.org/r/11281/#review10857

Ship It!

Updated

3 years ago
Attachment #8622239 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8622239 [details]
MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical

https://reviewboard.mozilla.org/r/11285/#review10859

Ship It!
https://reviewboard.mozilla.org/r/11301/#review10847

> In what situation can Bind be called and the ShadowLayerParent is already be connected to something?
> Currently we always call Bind on a layer that we just created but this change suggests otherwise. If this situation is actually possible, wouldn't it make sense to only disconnect if layer != mLayer ?

I don't know that Bind can be called with non-null mLayer. I was being paranoid because with the weak pointers I'm introducing here, failure to call mLayer->Disconnect would leave mLayer with a dangling pointer.

I'll wrap this in a layer != mLayer check if that helps.
Comment on attachment 8622245 [details]
MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical

Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical
Attachment #8622245 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8622267 [details]
MozReview Request: Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical

https://reviewboard.mozilla.org/r/11339/#review10919

Ship It!
Attachment #8622267 - Flags: review?(nical.bugzilla) → review+
https://reviewboard.mozilla.org/r/11335/#review9719

> OK, I'll do that.

Actually, this method doesn't need the lock count to be 1, so I'm just fixing the comment.
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Attachment #8622203 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Attachment #8622204 - Attachment description: MozReview Request: Bug 1143575. Resizing a WebGL canvas should trigger PresentScreenBuffer. r=jgilbert → MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical
Attachment #8622204 - Flags: review?(nical.bugzilla)
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Attachment #8622205 - Attachment description: MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical → MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Attachment #8622205 - Flags: review+ → review?(bent.mozilla)
Comment on attachment 8622206 [details]
MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce

Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Attachment #8622206 - Attachment description: MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent → MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce
Attachment #8622206 - Flags: review+ → review?(cpearce)
Comment on attachment 8622207 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical

Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical
Attachment #8622207 - Attachment description: MozReview Request: Bug 1143575. Add RefBase #include to stagefright stubs. r=cpearce → MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical
Attachment #8622207 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622208 [details]
MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical

Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical
Attachment #8622208 - Attachment description: MozReview Request: Bug 1143575. Add some #includes to avoid unified-build issues on Windows. r=nical → MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical
Attachment #8622208 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622209 [details]
MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce

Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce
Attachment #8622209 - Attachment description: MozReview Request: Bug 1143575. Add some #includes to avoid more unified-build issues on Windows. r=nical → MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce
Attachment #8622209 - Flags: review+ → review?(cpearce)
Comment on attachment 8622210 [details]
MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical

Bug 1143575. Make GL context current before cleaning up programs. r=nical

Otherwise we can get a crash with the following stack:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 14711]
0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
683	        MOZ_ASSERT(IsCurrent());
(gdb) where
#0  0x5d99974e in mozilla::gl::GLContext::BeforeGLCall (this=0x6dbf0800,
    funcName=0x60f251a4 <mozilla::gl::GLContext::raw_fDeleteProgram(unsigned int)::__PRETTY_FUNCTION__> "void mozilla::gl::GLContext::raw_fDeleteProgram(GLuint)") at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:683
#1  0x5d99bed6 in mozilla::gl::GLContext::raw_fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2232
#2  0x5d99c10a in mozilla::gl::GLContext::fDeleteProgram (this=0x6dbf0800, program=210003)
    at /home/roc/mozilla-inbound/gfx/gl/GLContext.h:2270
#3  0x5daa0ae6 in mozilla::layers::ShaderProgramOGL::~ShaderProgramOGL (this=0x6d7df000, __in_chrg=<optimized out>)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/OGLShaderProgram.cpp:491
#4  0x5da86bdc in mozilla::layers::CompositorOGL::CleanupResources (this=0x67ae4d70)
    at /home/roc/mozilla-inbound/gfx/layers/opengl/CompositorOGL.cpp:177
Attachment #8622210 - Attachment description: MozReview Request: Bug 1143575. test_HaveMetadataUnbufferedSeek should not wait for canplay since preload='metadata' elements may not fire canplay. r=cpearce → MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical
Attachment #8622210 - Flags: review+ → review?(nical.bugzilla)
Attachment #8622211 - Attachment description: MozReview Request: Bug 1143575. Make GL context current before cleaning up programs. r=nical → MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical
Attachment #8622211 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622211 [details]
MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical

There is some ambiguity about whether ScheduleComposite will necessarily
trigger a composite all the way to nsWindow::DrawWindowUnderlay. Android
robocop tests assume it will, because they rely on DrawWindowOverlay
being called so they can take a screenshot and make progress,
but this is a very fragile assumption. They also rely on the entire
window being painted, which is also a fragile assumption.

This patch improves the situation by explicitly invalidating the current
window area when Android Java code needs to trigger a composite. This avoids
regressions from future patches in this series which make composition bail
out when there is nothing invalid.

The resulting setup is still a bit fragile for my taste but I'm not sure
what the ideal solution would be.
Attachment #8622212 - Attachment description: MozReview Request: Bug 1143575. Android's screenshotting code should invalidate the LayerManagerComposite to ensure composition will actually happen. r=nical → MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical
Attachment #8622212 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622212 [details]
MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical
Attachment #8622213 - Attachment description: MozReview Request: Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical → MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical
Attachment #8622213 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused CompositionNotifySink. r=nical
Attachment #8622214 - Attachment description: MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical → MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical
Attachment #8622214 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622214 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical
Attachment #8622215 - Attachment description: MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical → MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical
Attachment #8622215 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622215 [details]
MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical
Attachment #8622216 - Attachment description: MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical → MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical
Attachment #8622216 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622216 [details]
MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical
Attachment #8622217 - Attachment description: MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical → MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical
Attachment #8622217 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622217 [details]
MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical
Attachment #8622218 - Attachment description: MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical → MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical
Attachment #8622218 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622218 [details]
MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Bug 1143575. Remove unused ReturnReleaseFence. r=nical
Attachment #8622219 - Attachment description: MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical → MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow
Attachment #8622219 - Flags: review+ → review?(matt.woodrow)
Comment on attachment 8622219 [details]
MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow
Attachment #8622220 - Attachment description: MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow → MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical
Attachment #8622220 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622220 [details]
MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical
Comment on attachment 8622221 [details]
MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical
Attachment #8622221 - Attachment description: MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical → MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical
Attachment #8622221 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622222 [details]
MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

This simplifies code slightly.
Attachment #8622222 - Attachment description: MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical → MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical
Attachment #8622222 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622223 [details]
MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical

Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical
Attachment #8622223 - Attachment description: MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical → MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical
Attachment #8622223 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622224 [details]
MozReview Request: Bug 1143575. Fix indent. r=cpearce

Bug 1143575. Fix indent. r=cpearce
Attachment #8622224 - Attachment description: MozReview Request: Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical → MozReview Request: Bug 1143575. Fix indent. r=cpearce
Attachment #8622224 - Flags: review+ → review?(cpearce)
Attachment #8622225 - Attachment description: MozReview Request: Bug 1143575. Fix indent. r=cpearce → MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce
Attachment #8622225 - Flags: review+ → review?(cpearce)
Comment on attachment 8622225 [details]
MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce
Attachment #8622226 - Attachment description: MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce → MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce
Attachment #8622226 - Flags: review+ → review?(cpearce)
Comment on attachment 8622226 [details]
MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce
Attachment #8622227 - Attachment description: MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce → MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce
Attachment #8622227 - Flags: review+ → review?(cpearce)
Comment on attachment 8622227 [details]
MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce

This makes MediaDecoderStateMachine::GetVideoStreamPosition compute a
time that's more consistent with the audio clock.
Attachment #8622228 - Attachment description: MozReview Request: Bug 1143575. Make GetClock return a TimeStamp as well as the stream time. r=cpearce → MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce
Attachment #8622228 - Flags: review+ → review?(cpearce)
Comment on attachment 8622228 [details]
MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce

Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce
Comment on attachment 8622229 [details]
MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce

Bug 1143575. Rename clock_time to clockTime. r=cpearce
Attachment #8622229 - Attachment description: MozReview Request: Bug 1143575. ScheduleStateMachine when the playback rate changes, so we can update the rendered frame queue. r=cpearce → MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce
Attachment #8622229 - Flags: review+ → review?(cpearce)
Comment on attachment 8622230 [details]
MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce

This makes normal playback consistent with the buffering state, which already
does this. We'll also need this when we handle multiple images, because then
we need to hande the entire queue of images to the ImageContainer without
pulling any of them off the queue.
Attachment #8622230 - Attachment description: MozReview Request: Bug 1143575. Rename clock_time to clockTime. r=cpearce → MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce
Attachment #8622230 - Flags: review+ → review?(cpearce)
Comment on attachment 8622231 [details]
MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce

Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce
Attachment #8622231 - Attachment description: MozReview Request: Bug 1143575. Keep currently-rendered frame at the front of the video queue. r=cpearce → MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce
Attachment #8622231 - Flags: review?(cpearce)
Comment on attachment 8622232 [details]
MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical

The picture rect logically belongs with the texture, and later patches will
make OpUseTexture take multiple textures, each of which needs its own
picture rect.
Attachment #8622232 - Attachment description: MozReview Request: Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce → MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical
Attachment #8622232 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622203 [details]
MozReview Request: Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical

Bug 1143575. #include nsDebug.h in YCbCrImageDataSerializer.cpp for NS_WARN_IF. r=nical
Comment on attachment 8622233 [details]
MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical

Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical
Attachment #8622233 - Attachment description: MozReview Request: Bug 1143575. Pass a picture rect with OpUseOverlaySource and OpUseTexture, and eliminate OpUpdatePictureRect. r=nical → MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical
Attachment #8622233 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622204 [details]
MozReview Request: Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

Bug 1143575. Avoid including Android's GraphicBuffer.h from LayersTypes.h. r=nical

On some Android versions, GraphicBuffer.h ends up including libui's
hardware.h, which #defines the symbols version_minor and version_major, which
are used as field names in Ogg Theora's th_info struct. Later patches will
require some files to include both Theora headers and LayerTypes.h.
Comment on attachment 8622234 [details]
MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical

Bug 1143575. Fix typo in ImageContainer comment. r=nical
Attachment #8622234 - Attachment description: MozReview Request: Bug 1143575. Rename ImageBridgeChild's AutoRemoteTextures to AutoRemoveTexturesFromImageBridge to avoid clashes with later work. r=nical → MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical
Attachment #8622234 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622205 [details]
MozReview Request: Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent

Bug 1143575. Avoid use of COMPARE macro which can clash with Android headers. r=bent
Comment on attachment 8622235 [details]
MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical

Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical
Attachment #8622235 - Attachment description: MozReview Request: Bug 1143575. Fix typo in ImageContainer comment. r=nical → MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical
Attachment #8622235 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622236 [details]
MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical

Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical
Attachment #8622236 - Attachment description: MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical → MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical
Attachment #8622236 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622237 [details]
MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical

Bug 1143575. Store composition time in Compositor. r=nical

We'll need this later so ImageHost can select the correct image to use.

Adding a TimeStamp parameter to BeginFrame is a bit annoying since BeginFrame
is overridden by every subclass. It's a bit more convenient to just call a
separate non-virtual method just before we call BeginFrame.
Attachment #8622237 - Attachment description: MozReview Request: Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical → MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical
Attachment #8622237 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622238 [details]
MozReview Request: Bug 1143575. Implement ImageHost support for multiple timed images. r=nical

Bug 1143575. Implement ImageHost support for multiple timed images. r=nical
Attachment #8622238 - Attachment description: MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical → MozReview Request: Bug 1143575. Implement ImageHost support for multiple timed images. r=nical
Comment on attachment 8622239 [details]
MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical

Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical
Attachment #8622239 - Attachment description: MozReview Request: Bug 1143575. Implement ImageHost support for multiple timed images. r=nical → MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical
Attachment #8622239 - Flags: review+ → review?(nical.bugzilla)
Attachment #8622240 - Attachment description: MozReview Request: Bug 1143575. Ensure we schedule another composite if ImageHost has pending images. r=nical → MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical
Attachment #8622240 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622240 [details]
MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical

When ImageContainer and ImageClient are managing a list of images, the
individual Image serial numbers are no longer enough to detect whether the
state has changed.
Attachment #8622241 - Attachment description: MozReview Request: Bug 1143575. Replace ImageClientSingle::UpdateImage's use of Image serial numbers with ImageContainer state generation counters, and switch it to use ImageContainer::GetCurrentImages. r=nical → MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical
Attachment #8622241 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

Bug 1143575. Remove ImageClientBridge::Updated. r=nical
Attachment #8622242 - Attachment description: MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical → MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical
Attachment #8622242 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622242 [details]
MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical

Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical
Comment on attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

Bug 1143575. Fix some code formatting. r=nical
Attachment #8622243 - Attachment description: MozReview Request: Bug 1143575. ImageClient::UpdateImage should not return false when there's no image, because recreating the ImageClient won't help. r=nical → MozReview Request: Bug 1143575. Fix some code formatting. r=nical
Attachment #8622243 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622244 [details]
MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro

Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro
Attachment #8622244 - Attachment description: MozReview Request: Bug 1143575. Fix some code formatting. r=nical → MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical,sotaro
Attachment #8622244 - Flags: review?(sotaro.ikeda.g)
Attachment #8622244 - Flags: review?(nical.bugzilla)
Attachment #8622244 - Flags: review+
Attachment #8622245 - Attachment description: MozReview Request: Bug 1143575. Factor out AsyncTransactionWaiter from AsyncTransactionTracker so we'll be able to wait for multiple AsyncTransactionTrackers. r=nical → MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical
Attachment #8622245 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8622245 [details]
MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical

Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical
Attachment #8622246 - Attachment description: MozReview Request: Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical → MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical
Attachment #8622246 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622246 [details]
MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

For frame statistics to work properly, we have to notify an ImageContainer
when it has been composited. This requires a few changes, which have
been lumped together in this patch:
-- Create PImageContainer and ImageContainerParent/ImageContainerChild.
-- Add mFrameID and mProducerID everywhere we're passing around images.
-- Route composition notifications from the compositor back to
ImageContainerChild.
Comment on attachment 8622247 [details]
MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow

Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow
Attachment #8622247 - Attachment description: MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical → MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow
Attachment #8622247 - Flags: review?(matt.woodrow)
Comment on attachment 8622248 [details]
MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow

We need this change so that when ImageHost has a next image to display
more than one composition-interval in the future, we skip the actual
compositing work in those intermediate composition(s) if nothing else
has changed.

This change is a little bit scary since it breaks any code that was
previously assuming ScheduleComposition would actually update the screen.
However, that code was already broken for BasicCompositor.
Attachment #8622248 - Attachment description: MozReview Request: Bug 1143575. Make LayerTreeInvalidation invalidate when an ImageLayerComposite's current frame has changed. r=mattwoodrow → MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow
Attachment #8622248 - Flags: review+ → review?(matt.woodrow)
Comment on attachment 8622249 [details]
MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow

We need to remove this so that adding images to the end of the list of images
for an ImageLayer doesn't force composition to happen even if nothing else
has changed.
Attachment #8622249 - Attachment description: MozReview Request: Bug 1143575. Exit composition early if nothing is invalid. r=mattwoodrow → MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow
Attachment #8622249 - Flags: review+ → review?(matt.woodrow)
Attachment #8622250 - Attachment description: MozReview Request: Bug 1143575. Async image invalidation does not necessarily need to invalidate the layer; LayerTreeInvalidation will do that for us. r=mattwoodrow → MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical
Attachment #8622250 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622250 [details]
MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical

Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical
Comment on attachment 8622251 [details]
MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce

Bug 1143575. Don't report negative frame delays. r=cpearce
Attachment #8622251 - Attachment description: MozReview Request: Bug 1143575. Pass a list of timestamped images to ImageContainer::SetCurrentImages. r=nical → MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce
Attachment #8622251 - Flags: review+ → review?(cpearce)
Attachment #8622252 - Attachment description: MozReview Request: Bug 1143575. Don't report negative frame delays. r=cpearce → MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical
Attachment #8622252 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622252 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical
Comment on attachment 8622253 [details]
MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

ImageBridgeChild::FlushAllImages with aExceptFront==true does absolutely
nothing, so remove the parameter and remove all callers which pass true.
Attachment #8622253 - Attachment description: MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical → MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical
Attachment #8622253 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622255 [details]
MozReview Request: Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical

Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical

We need to make it clear that ClearCurrentImage is really an internal method
of the ImageContainer implementation, not a method that ImageContainer users
should call.
Attachment #8622255 - Attachment description: MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical → MozReview Request: Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical
Attachment #8622255 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622256 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical
Attachment #8622256 - Attachment description: MozReview Request: Bug 1143575. Clarify code by renaming method to ClearCurrentImageFromImageBridge. r=nical → MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical
Attachment #8622256 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622257 [details]
MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical

Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical
Attachment #8622257 - Attachment description: MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical → MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical
Attachment #8622257 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622258 [details]
MozReview Request: Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical

Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical
Attachment #8622258 - Attachment description: MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical → MozReview Request: Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical
Attachment #8622258 - Flags: review+ → review?(nical.bugzilla)
Attachment #8622259 - Attachment description: MozReview Request: Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical → MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical
Attachment #8622259 - Flags: review+ → review?(nical.bugzilla)
Comment on attachment 8622259 [details]
MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical

Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical
Attachment #8622260 - Attachment description: MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical → MozReview Request: Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce
Attachment #8622260 - Flags: review+ → review?(cpearce)
Comment on attachment 8622260 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce
Comment on attachment 8622261 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce
Attachment #8622261 - Attachment description: MozReview Request: Bug 1143575. Introduce VideoFrameContainer::ClearCurrentFrame(size), and don't increment mFrameID when clearing frames. r=cpearce → MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce
Attachment #8622261 - Flags: review+ → review?(cpearce)
Attachment #8622262 - Attachment description: MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce → MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce
Attachment #8622262 - Flags: review+ → review?(cpearce)
Comment on attachment 8622262 [details]
MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce
Attachment #8622263 - Attachment description: MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce → MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce
Attachment #8622263 - Flags: review+ → review?(cpearce)
Comment on attachment 8622263 [details]
MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce

Bug 1143575. Add frame IDs to VideoData. r=cpearce
Attachment #8622264 - Attachment description: MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce → MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce
Attachment #8622264 - Flags: review+ → review?(cpearce)
Comment on attachment 8622264 [details]
MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce

Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce
Attachment #8622265 - Attachment description: MozReview Request: Bug 1143575. Refactor UpdateRenderedVideoFrames to support pushing multiple frames from the VideoQueue to the ImageContainer. r=cpearce → MozReview Request: Bug 1143575. Push all available frames to the compositor. r=cpearce
Attachment #8622265 - Flags: review?(cpearce)
Comment on attachment 8622265 [details]
MozReview Request: Bug 1143575. Push all available frames to the compositor. r=cpearce

Bug 1143575. Push all available frames to the compositor. r=cpearce