1143575 - Send timestamped images from video decoding to the compositor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

WIP here: https://bitbucket.org/rocallahan/mozilla-inbound/

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, didn't mean to needinfo.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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? :-)

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

#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).

Comment 8

[Mason Chang [Inactive] [:mchang]]

(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!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 10

[Mason Chang [Inactive] [:mchang]]

(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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=163b7a27ee9d

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11da2cf71a96

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, I'm green on Android now, after wasting time on bug 1174072 :-(.

Got a couple of failures on Windows to go:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-14cbe5450224/try-win32-debug/try_win7-ix-debug_test-mochitest-3-bm119-tests1-windows-build1327.txt.gz
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-14cbe5450224/try-win32-debug/try_win7-ix-debug_test-mochitest-gl-bm111-tests1-windows-build834.txt.gz

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632a2a5906f3

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, with this push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4711f511ab8
and this fix on top of it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b84b37a7973
we are green.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Bug 1143575. Remove unused CompositionNotifySink. r=nical

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Bug 1143575. Remove unused VideoFrameContainer::ClearCurrentFrame aResetSize parameter. r=nical

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Bug 1143575. LayerManagerComposite can't get END_NO_COMPOSITE. r=mattwoodrow

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Move mLayer from ImageClientBridge up into its superclass ImageClient. r=nical

Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Fix indent. r=cpearce

Bug 1143575. Convert SetCurrentImage(nullptr) callers to call ClearAllImages instead. r=nical

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

Bug 1143575. Fix indent. r=cpearce

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

Bug 1143575. Remove Theora-only duplicate frame optimization. r=cpearce

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Replace ImageContainer Lock methods with simplified AutoLockImage. r=nical

Bug 1143575. Fix typo in ImageContainer comment. r=nical

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Store composition time in Compositor. r=nical

Bug 1143575. Extend IPDL OpUseTexture to support multiple timestamped images. r=nical

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

Bug 1143575. Make ImageClientSingle handle multiple textures. r=nical

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Bug 1143575. Don't report negative frame delays. r=cpearce

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Remove ClearAllImagesExceptFront because it doesn't do anything. r=nical

Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused Image::IsSentToCompositor tracking. r=nical

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused CompositionNotifySink. r=nical

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 97

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 103

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Fix indent. r=cpearce

Comment 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 105

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 111

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 112

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 113

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 114

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 115

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 117

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 118

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 119

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 120

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 121

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 124

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 125

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 126

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 127

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 128

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 132

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 137

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 139

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Reimplement ImageContainer::GetPaintCount to be composition-aware. r=nical

Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

Comment 140

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 141

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Let ImageContainer::SetCurrentImages accept multiple images. r=nical

Bug 1143575. Let callers of ImageContainer::SetCurrentImages specify frame IDs. r=nical

Comment 142

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 143

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 144

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

Comment 145

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1143575. Add frame IDs to VideoData. r=cpearce

Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Comment 146

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 147

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 148

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 149

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 150

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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".

Comment 151

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 152

[JW Wang [:jwwang] [:jw_wang]]

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

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

Could it be a const?

Comment 153

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 154

[JW Wang [:jwwang] [:jw_wang]]

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

> tab

Oops. this is not a tab but a mark from review board to indicate indent changes.

Comment 155

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 156

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 157

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 158

[Matt Woodrow (:mattwoodrow)]

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

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

Ship It!

Comment 159

[Matt Woodrow (:mattwoodrow)]

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 160

[Matt Woodrow (:mattwoodrow)]

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!

Comment 161

[Matt Woodrow (: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

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

Ship It!

Comment 162

[Matt Woodrow (:mattwoodrow)]

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!

Comment 163

[Nicolas Silva [:nical]]

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?

Comment 164

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

> else if?

Could be, but I think it's fine as is.

Comment 165

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure. I understand it's a lot of reviews :-)

Comment 166

[JW Wang [:jwwang] [:jw_wang]]

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 167

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 168

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

r+

Comment 169

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 170

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 171

[Kelsey Gilbert [:jgilbert]]

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.

Comment 172

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, tell me how to fix the bug properly.

Comment 173

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 174

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 175

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 176

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 177

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 178

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 179

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

https://reviewboard.mozilla.org/r/11259/#review10033

Comment 180

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 181

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 182

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 183

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 184

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 185

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 186

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622261 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

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

Ship It!

Comment 187

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622262 [details]
MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

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

Ship It!

Comment 188

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 189

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 190

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 191

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 192

[Nicolas Silva [:nical]]

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!

Comment 193

[Nicolas Silva [:nical]]

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!

Comment 194

[Nicolas Silva [:nical]]

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!

Comment 195

[Nicolas Silva [: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

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

Ship It!

Comment 196

[Nicolas Silva [:nical]]

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 197

[Nicolas Silva [:nical]]

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!

Comment 198

[Nicolas Silva [:nical]]

Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

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

Ship It!

Comment 199

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 200

[Nicolas Silva [:nical]]

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 201

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 202

[Nicolas Silva [:nical]]

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!

Comment 203

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 204

[Nicolas Silva [:nical]]

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 205

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 206

[Nicolas Silva [:nical]]

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 207

[Nicolas Silva [:nical]]

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!

Comment 208

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 209

[Nicolas Silva [:nical]]

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!

Comment 210

[Nicolas Silva [:nical]]

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!

Comment 211

[Nicolas Silva [:nical]]

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!

Comment 212

[Nicolas Silva [:nical]]

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!

Comment 213

[Nicolas Silva [:nical]]

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 214

[Nicolas Silva [: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

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

Ship It!

Comment 215

[Nicolas Silva [:nical]]

Comment on attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

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

Ship It!

Comment 216

[Nicolas Silva [:nical]]

Comment on attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

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

Ship It!

Comment 217

[Nicolas Silva [:nical]]

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!

Comment 218

[Nicolas Silva [:nical]]

Comment on attachment 8622246 [details]
MozReview Request: Bug 1143575. Route ImageCompositeNotifications to ImageContainers. r=nical

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

Ship It!

Comment 219

[Nicolas Silva [:nical]]

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 220

[Nicolas Silva [:nical]]

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

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

Ship It!

Comment 221

[Nicolas Silva [:nical]]

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

Comment 222

[Nicolas Silva [:nical]]

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!

Comment 223

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 224

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 225

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 226

[Nicolas Silva [:nical]]

(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?

Comment 227

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, I'll do that.

Comment 228

[Nicolas Silva [:nical]]

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!

Comment 229

[Nicolas Silva [:nical]]

Comment on attachment 8622256 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

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

Ship It!

Comment 230

[Nicolas Silva [:nical]]

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!

Comment 231

[Nicolas Silva [:nical]]

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.

Comment 232

[Nicolas Silva [:nical]]

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 ?

Comment 233

[Nicolas Silva [:nical]]

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!

Comment 234

[Nicolas Silva [:nical]]

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.

Comment 235

[Nicolas Silva [:nical]]

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!

Comment 236

[Nicolas Silva [:nical]]

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!

Comment 237

[Nicolas Silva [:nical]]

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!

Comment 238

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 239

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 240

[Nicolas Silva [:nical]]

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!

Comment 241

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 242

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 243

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 244

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 245

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 246

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 247

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 248

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 249

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 250

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 251

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 252

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622213 [details]
MozReview Request: Bug 1143575. Remove unused CompositionNotifySink. r=nical

Bug 1143575. Remove unused CompositionNotifySink. r=nical

Comment 253

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused VideoFrameContainer::Reset. r=nical

Comment 254

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Rename mAsyncTransactionTrackeres to mAsyncTransactionTrackers. r=nical

Comment 255

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused ImageContainer::ResetPaintCount. r=nical

Comment 256

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 257

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused ReturnReleaseFence. r=nical

Comment 258

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 259

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused AttachAsyncCompositable overload. r=nical

Comment 260

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused CompositableClient::OnTransaction. r=nical

Comment 261

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 262

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 263

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Fix indent. r=cpearce

Comment 264

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 265

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Rename AdvanceFrame to UpdateRenderedVideoFrames. r=cpearce

Comment 266

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 267

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 268

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 269

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 270

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Remove unused MediaQueue::Empty. r=cpearce

Comment 271

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 272

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 273

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 274

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 275

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 276

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 277

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 278

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 279

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 280

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 281

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 282

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 283

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622241 [details]
MozReview Request: Bug 1143575. Remove ImageClientBridge::Updated. r=nical

Bug 1143575. Remove ImageClientBridge::Updated. r=nical

Comment 284

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 285

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622243 [details]
MozReview Request: Bug 1143575. Fix some code formatting. r=nical

Bug 1143575. Fix some code formatting. r=nical

Comment 286

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 287

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 288

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 289

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 290

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 291

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 292

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 293

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 294

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Implement ImageContainer::GetPaintDelay. r=nical

Comment 295

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 296

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 297

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622256 [details]
MozReview Request: Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

Bug 1143575. Implement ImageContainer::GetDroppedCount. r=nical

Comment 298

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 299

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 300

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 301

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 302

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622261 [details]
MozReview Request: Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

Bug 1143575. Introduce VideoFrameContainer::SetCurrentFrames. r=cpearce

Comment 303

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8622262 [details]
MozReview Request: Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Bug 1143575. Add MediaQueue::GetFirstElements. r=cpearce

Comment 304

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 305

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 306

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 307

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Bug 1143575. Add a bias value to ImageHost to avoid unpredictable results when image times and compositor times are closely aligned. r=nical

Comment 308

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think for now we should just ignore the Bugzilla r? state and get updated reviews for the patches not already ticked green in https://reviewboard.mozilla.org/r/11211/.

Comment 309

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 310

[Nicolas Silva [: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

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

Ship It!

Comment 311

[Nicolas Silva [:nical]]

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/#review11057

Ship It!

Comment 312

[Nicolas Silva [:nical]]

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

https://reviewboard.mozilla.org/r/11283/#review11059

Ship It!

Comment 313

[Nicolas Silva [:nical]]

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

https://reviewboard.mozilla.org/r/11297/#review11061

Ship It!

Comment 314

[Matt Woodrow (:mattwoodrow)]

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/#review11067

Ship It!

Comment 315

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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/#review11075

Ship It!

Comment 316

[Sotaro Ikeda [:sotaro]]

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

Looks good.

Comment 317

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

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

Ship It!

Comment 318

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622265 [details]
MozReview Request: Bug 1143575. Push all available frames to the compositor. r=cpearce

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

Ship It!

Comment 319

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eaf88c0ceb2

Comment 320

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 321

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 322

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Actually even calling SetCompositionTime just before BeginFrame broke things. We need to set the composition time before we call ComputeEffectiveTransforms, since ImageHost must get the correct composition time to choose the right frame to render so it computes the transform needed to stretch that frame to the required size. So I moved SetCompositionTime to just before we call ComputeEffectiveTransforms in EndTransaction.

I have added a few other trivial patches to fix rot.

Comment 323

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e14d68e1e4a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba169610eb8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7002e70d949
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31af82d8b9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/47592e40ee7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b05b5b0808e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c68f224513a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b745c996a71d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3527e6013de9
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecf73959121
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61363797ed6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6f54bca784
https://hg.mozilla.org/integration/mozilla-inbound/rev/b88e4fdc2033
https://hg.mozilla.org/integration/mozilla-inbound/rev/64f323051975
https://hg.mozilla.org/integration/mozilla-inbound/rev/38a97a683d65
https://hg.mozilla.org/integration/mozilla-inbound/rev/1494a4e9ab4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2b4b9c9571
https://hg.mozilla.org/integration/mozilla-inbound/rev/174624481eb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72d2ec123b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c119db5addfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/e306e8d40085
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd5bc5beb76
https://hg.mozilla.org/integration/mozilla-inbound/rev/175dd79bf104
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1be20110d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d318eb52845
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c82d4f1b34a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5ee8e49c60
https://hg.mozilla.org/integration/mozilla-inbound/rev/3577cfb4353f
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc00000dd20
https://hg.mozilla.org/integration/mozilla-inbound/rev/780ea52b2d36
https://hg.mozilla.org/integration/mozilla-inbound/rev/30fbb3f4be2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb9996b1cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e587e6bd4d6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/44927f617616
https://hg.mozilla.org/integration/mozilla-inbound/rev/03a8e48f4134
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7462889f82f
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f44f63b6a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/98cece19d4fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe6ff5de12e
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b18fa3141b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1433100e89d
https://hg.mozilla.org/integration/mozilla-inbound/rev/15bc0858e2be
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2ed2e93f49
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee029e39ce4
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ee1bf0f982
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d10765f4a82
https://hg.mozilla.org/integration/mozilla-inbound/rev/73e94ff36e19
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23a779852ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/bea2adc0a077
https://hg.mozilla.org/integration/mozilla-inbound/rev/84481760a4de
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf230e06ace0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c12696522b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1edd02746cd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/057cca533e0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5aab92845f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced4d3f1a189
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8932f93615
https://hg.mozilla.org/integration/mozilla-inbound/rev/8adcb7e36c7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a851d6125035
https://hg.mozilla.org/integration/mozilla-inbound/rev/8679a837bb3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc557cdb8c1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec0906f885c
https://hg.mozilla.org/integration/mozilla-inbound/rev/30b267369242
https://hg.mozilla.org/integration/mozilla-inbound/rev/a082d7f6cfa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7cb62b5b23
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10665af6c50

Comment 324

[Nicolas Silva [:nical]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline July 8-10) from comment #322)
> > 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.
> 
> Actually even calling SetCompositionTime just before BeginFrame broke
> things. We need to set the composition time before we call
> ComputeEffectiveTransforms, since ImageHost must get the correct composition
> time to choose the right frame to render so it computes the transform needed
> to stretch that frame to the required size. So I moved SetCompositionTime to
> just before we call ComputeEffectiveTransforms in EndTransaction.

Fair enough. If there are (non-trivial) dependencies between the order of method calls please explicit them in a comment. Having a SetCompositiongTime method is not my favourite style but it's not the end of the world :)

Comment 325

[Julien Wajsberg [:julienw]]

Hi,

in B2G Desktop (I haven't tried B2G) we don't get the "error" event in case a video does not load.

Testcase: https://everlong.org/mozilla/test-video-error.html

Comment 326

[Julien Wajsberg [:julienw]]

Mmm sorry, it looks like I don't get the error event in previous versions either. Still investigating why this makes bug 1108507 not intermittent then...

Comment 327

[Julien Wajsberg [:julienw]]

I don't know yet if this is related to the issue we have in the unit tests, but I noticed that we have an invalidation problem.

For example load any video inside Firefox or B2G with your patch:
http://techslides.com/demos/sample-videos/small.webm

The video does not update properly with the patch. It updates if I move the mouse cursor on top of the video.

(I'm on Linux, in case this is useful).

Comment 328

[Ryan VanderMeulen [:RyanVM]]

Bug 1181201 filed to track the issues with tutorial_test.js.

Comment 329

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

https://hg.mozilla.org/mozilla-central/rev/e14d68e1e4a3
https://hg.mozilla.org/mozilla-central/rev/ba169610eb8f
https://hg.mozilla.org/mozilla-central/rev/c7002e70d949
https://hg.mozilla.org/mozilla-central/rev/c31af82d8b9d
https://hg.mozilla.org/mozilla-central/rev/47592e40ee7e
https://hg.mozilla.org/mozilla-central/rev/b05b5b0808e9
https://hg.mozilla.org/mozilla-central/rev/4c68f224513a
https://hg.mozilla.org/mozilla-central/rev/b745c996a71d
https://hg.mozilla.org/mozilla-central/rev/3527e6013de9
https://hg.mozilla.org/mozilla-central/rev/6ecf73959121
https://hg.mozilla.org/mozilla-central/rev/a61363797ed6
https://hg.mozilla.org/mozilla-central/rev/5e6f54bca784
https://hg.mozilla.org/mozilla-central/rev/b88e4fdc2033
https://hg.mozilla.org/mozilla-central/rev/64f323051975
https://hg.mozilla.org/mozilla-central/rev/38a97a683d65
https://hg.mozilla.org/mozilla-central/rev/1494a4e9ab4f
https://hg.mozilla.org/mozilla-central/rev/7a2b4b9c9571
https://hg.mozilla.org/mozilla-central/rev/174624481eb3
https://hg.mozilla.org/mozilla-central/rev/e72d2ec123b4
https://hg.mozilla.org/mozilla-central/rev/c119db5addfe
https://hg.mozilla.org/mozilla-central/rev/e306e8d40085
https://hg.mozilla.org/mozilla-central/rev/2fd5bc5beb76
https://hg.mozilla.org/mozilla-central/rev/175dd79bf104
https://hg.mozilla.org/mozilla-central/rev/1a1be20110d8
https://hg.mozilla.org/mozilla-central/rev/7d318eb52845
https://hg.mozilla.org/mozilla-central/rev/7c82d4f1b34a
https://hg.mozilla.org/mozilla-central/rev/2c5ee8e49c60
https://hg.mozilla.org/mozilla-central/rev/3577cfb4353f
https://hg.mozilla.org/mozilla-central/rev/edc00000dd20
https://hg.mozilla.org/mozilla-central/rev/780ea52b2d36
https://hg.mozilla.org/mozilla-central/rev/30fbb3f4be2e
https://hg.mozilla.org/mozilla-central/rev/9eb9996b1cd4
https://hg.mozilla.org/mozilla-central/rev/e587e6bd4d6f
https://hg.mozilla.org/mozilla-central/rev/44927f617616
https://hg.mozilla.org/mozilla-central/rev/03a8e48f4134
https://hg.mozilla.org/mozilla-central/rev/c7462889f82f
https://hg.mozilla.org/mozilla-central/rev/62f44f63b6a5
https://hg.mozilla.org/mozilla-central/rev/98cece19d4fe
https://hg.mozilla.org/mozilla-central/rev/1fe6ff5de12e
https://hg.mozilla.org/mozilla-central/rev/11b18fa3141b
https://hg.mozilla.org/mozilla-central/rev/d1433100e89d
https://hg.mozilla.org/mozilla-central/rev/15bc0858e2be
https://hg.mozilla.org/mozilla-central/rev/3b2ed2e93f49
https://hg.mozilla.org/mozilla-central/rev/8ee029e39ce4
https://hg.mozilla.org/mozilla-central/rev/07ee1bf0f982
https://hg.mozilla.org/mozilla-central/rev/2d10765f4a82
https://hg.mozilla.org/mozilla-central/rev/73e94ff36e19
https://hg.mozilla.org/mozilla-central/rev/b23a779852ae
https://hg.mozilla.org/mozilla-central/rev/bea2adc0a077
https://hg.mozilla.org/mozilla-central/rev/84481760a4de
https://hg.mozilla.org/mozilla-central/rev/cf230e06ace0
https://hg.mozilla.org/mozilla-central/rev/2c12696522b7
https://hg.mozilla.org/mozilla-central/rev/1edd02746cd2
https://hg.mozilla.org/mozilla-central/rev/057cca533e0b
https://hg.mozilla.org/mozilla-central/rev/d5aab92845f1
https://hg.mozilla.org/mozilla-central/rev/ced4d3f1a189
https://hg.mozilla.org/mozilla-central/rev/aa8932f93615
https://hg.mozilla.org/mozilla-central/rev/8adcb7e36c7c
https://hg.mozilla.org/mozilla-central/rev/a851d6125035
https://hg.mozilla.org/mozilla-central/rev/8679a837bb3d
https://hg.mozilla.org/mozilla-central/rev/bc557cdb8c1b
https://hg.mozilla.org/mozilla-central/rev/5ec0906f885c
https://hg.mozilla.org/mozilla-central/rev/30b267369242
https://hg.mozilla.org/mozilla-central/rev/a082d7f6cfa2
https://hg.mozilla.org/mozilla-central/rev/ef7cb62b5b23
https://hg.mozilla.org/mozilla-central/rev/f10665af6c50