Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1265433003:
Preliminary change for new rtc rendering algorithm (Closed)
|
Created:
5 years, 4 months ago by qiangchen Modified:
5 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreliminary change for new rtc rendering algorithm:
1. Add trace flags for smoothness testing
2. Propagate render_time calculated in Jitter Buffer to media::VideoFrame for new rendering algorithm to use.
3. Move invalidation trigger to compositor thread.
BUG=514873
Committed: https://crrev.com/dde7ac68de3071061c47a1986a3344da0ffe1f7d
Cr-Commit-Position: refs/heads/master@{#344572}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Move render_time to metadata #
Total comments: 7
Patch Set 3 : TimeTicks generation change #Patch Set 4 : WebRTC Chromium Timestamp Alignment #
Total comments: 13
Patch Set 5 : Comment Fix #
Total comments: 4
Patch Set 6 : Comment fix #
Total comments: 6
Patch Set 7 : Pause and background rendering #Patch Set 8 : Use Compositor Thread to Start/Stop Rendering #
Total comments: 2
Patch Set 9 : Inner class #
Total comments: 26
Patch Set 10 : Style #
Total comments: 8
Patch Set 11 : Style #
Total comments: 24
Patch Set 12 : Style (Rebranched) #Patch Set 13 : Trybot #Patch Set 14 : Trybot #Patch Set 15 : Initial value issue #
Messages
Total messages: 101 (29 generated)
Patchset #1 (id:1) has been deleted
qiangchen@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:99: video_frame->set_render_time(base::TimeTicks::FromInternalValue( Using InternalValue like this isn't correct. Why do this instead of just setting the timestamp field directly? https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.... media/base/video_frame.h:379: // Record the ideal time instant to render this frame. VideoFrame is already in a crufty situation so we want to avoid adding any more one off fields like this. Can you deliver the render time through another mechanism? Either by set_timestamp(render_time - base::TimeTicks()) or via a new VideoFrameMetadata field?
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Drive-by. https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:456: } no need for {} https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: base::TimeTicks deadline_max) { Suggestion: change base::TimeTicks to base::TimeDelta and things will make more sense? https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:468: base::TimeTicks render_time = current_frame_->render_time(); const
https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: base::TimeTicks deadline_max) { On 2015/07/30 08:40:24, mcasas wrote: > Suggestion: change base::TimeTicks to base::TimeDelta and > things will make more sense? These are values forwarded from the compositor BeginFrameArgs structure and are relative to the values returned by base::TimeTicks::Now(), so they should be TimeTicks. Can you elaborate on your idea here?
Fixed and replied https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:456: } On 2015/07/30 08:40:24, mcasas wrote: > no need for {} Done. https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: base::TimeTicks deadline_max) { On 2015/07/30 08:40:24, mcasas wrote: > Suggestion: change base::TimeTicks to base::TimeDelta and > things will make more sense? Did you mean deadline_min and deadline_max? UpdateCurrentFrame is an overriden function from base class. If we change that, we need to change every implementation and every call. https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:468: base::TimeTicks render_time = current_frame_->render_time(); On 2015/07/30 08:40:24, mcasas wrote: > const Done. https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:99: video_frame->set_render_time(base::TimeTicks::FromInternalValue( On 2015/07/30 05:55:24, DaleCurtis wrote: > Using InternalValue like this isn't correct. Why do this instead of just setting > the timestamp field directly? cricket::VideoFrame's timestamp is a number in unit nanosecond. And here I need to convert it to a TimeTicks. What about "base::TimeTicks()+base::TimeDelta::FromMicroseconds(...)"? https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.... media/base/video_frame.h:379: // Record the ideal time instant to render this frame. On 2015/07/30 05:55:24, DaleCurtis wrote: > VideoFrame is already in a crufty situation so we want to avoid adding any more > one off fields like this. Can you deliver the render time through another > mechanism? Either by set_timestamp(render_time - base::TimeTicks()) or via a new > VideoFrameMetadata field? I'll consider using VideoFrameMetadata. Should I also remove the methods, and let the caller to retrieve information from metadata? P.S. Now the timestamp of media::VideoFrame is essentially the RTP timestamp/90000. While the render_time calculated in Jitter Buffer is derived by an equation fitting not from a static formula, which is necessary and powerful as it can kill the clock rate inconsistency. Thus we should use render_time as the guidance of frame selection. But I'm not sure if we simply set timestamp to be render_time, the loss of the raw timestamp would affect something else.
https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1265433003/diff/20001/media/base/video_frame.... media/base/video_frame.h:379: // Record the ideal time instant to render this frame. On 2015/07/30 17:40:26, qiangchenC wrote: > On 2015/07/30 05:55:24, DaleCurtis wrote: > > VideoFrame is already in a crufty situation so we want to avoid adding any > more > > one off fields like this. Can you deliver the render time through another > > mechanism? Either by set_timestamp(render_time - base::TimeTicks()) or via a > new > > VideoFrameMetadata field? > > I'll consider using VideoFrameMetadata. Should I also remove the methods, and > let the caller to retrieve information from metadata? > > P.S. > > Now the timestamp of media::VideoFrame is essentially the RTP timestamp/90000. > While the render_time calculated in Jitter Buffer is derived by an equation > fitting not from a static formula, which is necessary and powerful as it can > kill the clock rate inconsistency. > > Thus we should use render_time as the guidance of frame selection. But I'm not > sure if we simply set timestamp to be render_time, the loss of the raw timestamp > would affect something else. Before you go the metadata approach, I would see if you can just overwrite the existing timestamp() field at this point; it shouldn't be too hard to investigate the few call sites within WebRTC that use timestamp(). Definitely we don't want a new render_time() and set_render_time() on VideoFrame though; either you'll need to add a new VideoFrameMetadata field or stuff the value into the timestamp field (most ideal).
It sounds like what the WebRTC code calls "render_time" is the same thing Chrome media calls "estimated_capture_time," or in the media/cast library "reference_time." Either way, on the play-out/receiver side, it is meant to be interpreted as a *target* time on the timeline of the common reference clock, used for lip-sync'ing with the audio, and not an exact presentation time. The media timestamp, given by VideoFrame::timestamp() would be used to determine presentation times. Comments here explain this: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c... Another set of comments explaining pretty much the same thing a different way: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/net/cas... qiangchen, if you agree with this interpretation, then I agree with Dale that this would be a good thing to add to VideoFrameMetadata. I personally would prefer calling this REFERENCE_TIME. Once we have this in VideoFrameMetadata, we can do some clean-up CLs to remove a *lot* of timestamp arguments being passed in methods alongside VideoFrames throughout chrome/renderer/media and media/cast. :-)
miu@chromium.org changed reviewers: + miu@chromium.org
On 2015/07/30 18:29:10, miu wrote: > It sounds like what the WebRTC code calls "render_time" is the same thing Chrome > media calls "estimated_capture_time," or in the media/cast library > "reference_time." Either way, on the play-out/receiver side, it is meant to be > interpreted as a *target* time on the timeline of the common reference clock, > used for lip-sync'ing with the audio, and not an exact presentation time. The > media timestamp, given by VideoFrame::timestamp() would be used to determine > presentation times. > > Comments here explain this: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c... > > Another set of comments explaining pretty much the same thing a different way: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/net/cas... > > qiangchen, if you agree with this interpretation, then I agree with Dale that > this would be a good thing to add to VideoFrameMetadata. I personally would > prefer calling this REFERENCE_TIME. > > Once we have this in VideoFrameMetadata, we can do some clean-up CLs to remove a > *lot* of timestamp arguments being passed in methods alongside VideoFrames > throughout chrome/renderer/media and media/cast. :-) Yep, this explains a lot, and I agree with adding a Key to VideoFrameMetadata, rather than hijacking the timestamp field. For clean-up CLs, can you give me some more details? And are we going to do the clean up in this CL or create new ones? And Dale, what is your opinion?
On 2015/07/30 19:27:48, qiangchenC wrote: > Yep, this explains a lot, and I agree with adding a Key to VideoFrameMetadata, > rather than hijacking the timestamp field. Great! :) > For clean-up CLs, can you give me some more details? And are we going to do the > clean up in this CL or create new ones? In later CLs. We can introduce the new metadata field now and remove the interface duplication in one or more later changes (one for each major API/interface).
plan sgtm, miu@ and mcasas@ have done a lot more with VideoFrame recently and know the capture side well.
https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: base::TimeTicks deadline_max) { On 2015/07/30 17:40:26, qiangchenC wrote: > On 2015/07/30 08:40:24, mcasas wrote: > > Suggestion: change base::TimeTicks to base::TimeDelta and > > things will make more sense? > > Did you mean deadline_min and deadline_max? > UpdateCurrentFrame is an overriden function from base class. If we change that, > we need to change every implementation and every call. Yes, it would mean changing base class and derived classes. Perhaps even Blink, so might be too large a change for this CL (in this case, please add a // TODO() and create a crbug.com). My understanding was that base::TimeTicks is better avoided (http://crbug.com/467417), miu@ can comment on it some more, due to clock skews, and it can be transformed, nontrivially, into TimeDeltas from UTC.
https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: base::TimeTicks deadline_max) { On 2015/07/31 08:41:31, mcasas wrote: > On 2015/07/30 17:40:26, qiangchenC wrote: > > On 2015/07/30 08:40:24, mcasas wrote: > > > Suggestion: change base::TimeTicks to base::TimeDelta and > > > things will make more sense? > > > > Did you mean deadline_min and deadline_max? > > UpdateCurrentFrame is an overriden function from base class. If we change > that, > > we need to change every implementation and every call. > > Yes, it would mean changing base class and derived > classes. Perhaps even Blink, so might be too large a > change for this CL (in this case, please add a > // TODO() and create a http://crbug.com). > > My understanding was that base::TimeTicks is better > avoided (http://crbug.com/467417), miu@ can comment > on it some more, due to clock skews, and it can > be transformed, nontrivially, into TimeDeltas from > UTC. TimeTicks is the correct usage here since these values are the system clock times at which a frame would be displayed. They are used in conjunction with base::TimeTicks::Now() for scheduling.
Patchset #2 (id:40001) has been deleted
Can you take a look at Patch 2: 1. Move render_time to metadata; 2. Add some comment and TODO for timestamp. From miu's comment, not hijacking timestamp for target render time, as the media::VideoFrame::timestamp is defined as the elasped time since the first frame. Though functionally hijacking timestamp for render time would work for RTC, it would make the code less readable. https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:468: base::TimeTicks render_time = current_frame_->render_time(); On 2015/07/30 17:40:26, qiangchenC wrote: > On 2015/07/30 08:40:24, mcasas wrote: > > const > > Done. N/A
https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: TRACE_EVENT_BEGIN2("webrtc", "WebMediaPlayerMS::UpdateCurrentFrame", No need for BEGIN, END since your trace is within the same function, Just use TRACE_EVENT3 and it'll automatically capture the duration. https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:65: TRACE_EVENT_BEGIN0("webrtc", "RemoteVideoSourceDelegate::RenderFrame"); Ditto. https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:100: base::TimeTicks::FromInternalValue(incoming_frame->GetTimeStamp() / 1000); This still isn't correct unless the value returned by GetTimeStamp is based on the same clock as base::TimeTicks::Now(). Even then you should instead use base::TimeTicks() + base::TimeDelta::FromMicroseconds() then.
Fixed, replied, and one question on the same clock issue. https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:460: TRACE_EVENT_BEGIN2("webrtc", "WebMediaPlayerMS::UpdateCurrentFrame", On 2015/07/31 17:48:27, DaleCurtis wrote: > No need for BEGIN, END since your trace is within the same function, Just use > TRACE_EVENT3 and it'll automatically capture the duration. The point is that after implementing the algorithm, then at the beginning, we do not know what current_frame_ would be finally. By the way, it seems there isn't a macro called TRACE_EVENT3. https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:65: TRACE_EVENT_BEGIN0("webrtc", "RemoteVideoSourceDelegate::RenderFrame"); On 2015/07/31 17:48:27, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:100: base::TimeTicks::FromInternalValue(incoming_frame->GetTimeStamp() / 1000); On 2015/07/31 17:48:27, DaleCurtis wrote: > This still isn't correct unless the value returned by GetTimeStamp is based on > the same clock as base::TimeTicks::Now(). Even then you should instead use > base::TimeTicks() + base::TimeDelta::FromMicroseconds() then. This is a good point. The timestamp is from WebRTC, essentially webrtc::TickTime::Now(), is that the same with base::TimeTicks::Now()? In my testing, they are the same, but I'm not sure whether this may be platform specific. Or is it better to do a re-alignment here?
https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_remote_video_source.cc:100: base::TimeTicks::FromInternalValue(incoming_frame->GetTimeStamp() / 1000); On 2015/07/31 20:06:07, qiangchenC wrote: > On 2015/07/31 17:48:27, DaleCurtis wrote: > > This still isn't correct unless the value returned by GetTimeStamp is based on > > the same clock as base::TimeTicks::Now(). Even then you should instead use > > base::TimeTicks() + base::TimeDelta::FromMicroseconds() then. > > This is a good point. The timestamp is from WebRTC, essentially > webrtc::TickTime::Now(), is that the same with base::TimeTicks::Now()? In my > testing, they are the same, but I'm not sure whether this may be platform > specific. Or is it better to do a re-alignment here? If they're the same then using FromMicroseconds() + base::TimeTicks() is okay, but you should be sure they are the same -- otherwise yes you should do a realignment before exposing it outside of WebRTC like this.
Patchset #4 (id:100001) has been deleted
Do a WebRTC chromium timestamp alignment, when the WebRTC timestamp enters chromium code path. It seems there is no guarantee that webrtc::TickTime::Now is the same as base::TimeTicks::Now on all platforms.
Hmm, I defer to miu@ for correctness there. I'd guess that there's not a linear adjustment you can make for continuous correctness, but perhaps it's good enough for an instantaneous adjustment.
On 2015/07/31 08:41:31, mcasas wrote: > My understanding was that base::TimeTicks is better > avoided (http://crbug.com/467417), miu@ can comment > on it some more, due to clock skews, and it can > be transformed, nontrivially, into TimeDeltas from > UTC. Wait, there's a bit of confusion here. base::Time is the risky one. base::TimeTicks is a good "reference clock" for lip-sync'ing. base::TimeDelta is the preferred representation for frame timestamps in the media stream (and these would be the basis for computing presentation timestamps). There's a whole lot of background for why this is. I'll have to put together a design page about all this on the Chromium design doc site, as it is a very confusing subject that can be easily cleared up with some diagrams explaining how media recording/transmission/playback systems must deal with multiple clock sources.
https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... content/renderer/media/webrtc/media_stream_remote_video_source.cc:61: time_diff_us_ = Yes, this can be a tricky problem. There's two differences between clocks: 1) the offset, 2) the rate (i.e., one clock runs faster than the other). I would say this is an okay stub for now, but don't be surprised if there are performance issues on some platforms. I'd suggest two things: 1. Put a TODO comment here with a crbug to revisit this issue. 2. Add a SetRealTimeClock() function here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Then, wherever Chromium initializes libwebrtc, it could provide an implementation of the webrtc::Clock interface that is based on calling Chromium's base::TimeTicks::Now(). That way, ALL the code in both Chromium and WebRTC are using the same reference clock! :) https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame.h:368: // The time span between the current frame and the first frame of the stream. Nice. Please also add something to the effect of: "This is the media timestamp, and not the reference time. See VideoFrameMetadata::REFERENCE_TIME for details." https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame.h:369: // TODO(qiangchen): See above, move this into metadata. Let's not add this TODO yet. We should discuss with stakeholders whether the media timestamp should be moved into the metadata. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:71: // Render side: a TimeTicks that records the ideal time instant to render As this concept is confusing to a lot of media developers, can you provide more detail? FWIW, you could pretty much copy-and-paste the comment from here (with minor changes): https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c... https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:74: REFERENCE_TIME, Please move this up (for alphabetizing). https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:79: ELAPSED_TIME, Let's not add ELAPSED_TIME yet. We can do that in a future change, if we decide to move VideoFrame::timestamp() into the metadata.
Fix the comment, add TODO, remove unnecessary TODO. https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... content/renderer/media/webrtc/media_stream_remote_video_source.cc:61: time_diff_us_ = On 2015/08/04 04:35:07, miu wrote: > Yes, this can be a tricky problem. There's two differences between clocks: 1) > the offset, 2) the rate (i.e., one clock runs faster than the other). > > I would say this is an okay stub for now, but don't be surprised if there are > performance issues on some platforms. I'd suggest two things: > > 1. Put a TODO comment here with a crbug to revisit this issue. > > 2. Add a SetRealTimeClock() function here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Then, wherever Chromium initializes libwebrtc, it could provide an > implementation of the webrtc::Clock interface that is based on calling > Chromium's base::TimeTicks::Now(). That way, ALL the code in both Chromium and > WebRTC are using the same reference clock! :) Did the TODO. While, adding SetRealTimeClock() cannot be done in this CL, because that file is in WebRTC. And besides, in WebRTC, many places just refer to the static method TickTime::Now(), instead of GetRealTimeClock()->TimestampInMicrosecond(). Thus it is not that easy to make ALL the code use same clock. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame.h:368: // The time span between the current frame and the first frame of the stream. On 2015/08/04 04:35:07, miu wrote: > Nice. Please also add something to the effect of: "This is the media timestamp, > and not the reference time. See VideoFrameMetadata::REFERENCE_TIME for > details." Done. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame.h:369: // TODO(qiangchen): See above, move this into metadata. On 2015/08/04 04:35:08, miu wrote: > Let's not add this TODO yet. We should discuss with stakeholders whether the > media timestamp should be moved into the metadata. Done. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:71: // Render side: a TimeTicks that records the ideal time instant to render On 2015/08/04 04:35:08, miu wrote: > As this concept is confusing to a lot of media developers, can you provide more > detail? FWIW, you could pretty much copy-and-paste the comment from here (with > minor changes): > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c... Done. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:74: REFERENCE_TIME, On 2015/08/04 04:35:08, miu wrote: > Please move this up (for alphabetizing). Done. https://codereview.chromium.org/1265433003/diff/120001/media/base/video_frame... media/base/video_frame_metadata.h:79: ELAPSED_TIME, On 2015/08/04 04:35:08, miu wrote: > Let's not add ELAPSED_TIME yet. We can do that in a future change, if we decide > to move VideoFrame::timestamp() into the metadata. Done.
lgtm % one comment nit. https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/1265433003/diff/120001/content/renderer/media... content/renderer/media/webrtc/media_stream_remote_video_source.cc:61: time_diff_us_ = On 2015/08/04 16:35:25, qiangchenC wrote: > Did the TODO. While, adding SetRealTimeClock() cannot be done in this CL, Oh, yeah. I did mean in another CL. ;) > because that file is in WebRTC. And besides, in WebRTC, many places just refer > to the static method TickTime::Now(), instead of > GetRealTimeClock()->TimestampInMicrosecond(). Thus it is not that easy to make > ALL the code use same clock. I'd argue this is something important to fix in WebRTC, so I hope you or someone else will pursue it aggressively. The ability to use the SimulatedClock for simulations is a huge win. We did this for Cast Streaming a while back, and can easily test algorithm changes on our desktops by simulating a 5-minute session that only takes about 20 seconds in real-word time to run. https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... media/base/video_frame_metadata.h:55: // of the frame, if it was generated from a remote source. Please also add: "This value is NOT a high-resolution timestamp, and so it should not be used as a presentation time; but, instead, it should be used for buffering playback and for A/V synchronization purposes."
https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... media/base/video_frame_metadata.h:56: REFERENCE_TIME, Oh, also please mention "Use Get/SetTimeTicks() for this key." as we do for the other enums here.
https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... media/base/video_frame_metadata.h:55: // of the frame, if it was generated from a remote source. On 2015/08/04 20:37:32, miu wrote: > Please also add: "This value is NOT a high-resolution timestamp, and so it > should not be used as a presentation time; but, instead, it should be used for > buffering playback and for A/V synchronization purposes." Done. https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... media/base/video_frame_metadata.h:56: REFERENCE_TIME, On 2015/08/04 20:38:19, miu wrote: > Oh, also please mention "Use Get/SetTimeTicks() for this key." as we do for the > other enums here. Done.
On 2015/08/04 20:56:13, qiangchenC wrote: > https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... > File media/base/video_frame_metadata.h (right): > > https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... > media/base/video_frame_metadata.h:55: // of the frame, if it was generated from > a remote source. > On 2015/08/04 20:37:32, miu wrote: > > Please also add: "This value is NOT a high-resolution timestamp, and so it > > should not be used as a presentation time; but, instead, it should be used for > > buffering playback and for A/V synchronization purposes." > > Done. > > https://codereview.chromium.org/1265433003/diff/140001/media/base/video_frame... > media/base/video_frame_metadata.h:56: REFERENCE_TIME, > On 2015/08/04 20:38:19, miu wrote: > > Oh, also please mention "Use Get/SetTimeTicks() for this key." as we do for > the > > other enums here. > > Done. Thanks. Still lgtm.
https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); Where are you calling StopRendering? How are you handling backgrounding of the tab? If you turn this path on always, you won't get UpdateCurrentFrame calls when the layer is invisible.
replied with my plan for background rendering. https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); On 2015/08/04 21:49:26, DaleCurtis wrote: > Where are you calling StopRendering? How are you handling backgrounding of the > tab? If you turn this path on always, you won't get UpdateCurrentFrame calls > when the layer is invisible. video_frame_provider_client_->StopUsingProvider() will call StopRendering(). For background rendering, I found VideoFrameCompositor uses a background_rendering_timer_ to do background rendering when UpdateCurrentFrame does not get called for a period of time. In WebMediaPlayerMS, I think there is a simpler solution: just record the last deadline_max passed in UpdateCurrentFrame, when a new frame comes to OnFrameAvailable (which never stops even when the tab is invisible), if Now>last recorded deadline_max, then we call VRA::RemoveExpiredFrames to let old frames go.
dalecurtis@chromium.org changed reviewers: + sunnyps@chromium.org
+sunnyps in case he wants to add anything about StartRendering(). https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); On 2015/08/04 22:13:24, qiangchenC wrote: > On 2015/08/04 21:49:26, DaleCurtis wrote: > > Where are you calling StopRendering? How are you handling backgrounding of the > > tab? If you turn this path on always, you won't get UpdateCurrentFrame calls > > when the layer is invisible. > > video_frame_provider_client_->StopUsingProvider() will call StopRendering(). > > For background rendering, I found VideoFrameCompositor uses a > background_rendering_timer_ to do background rendering when UpdateCurrentFrame > does not get called for a period of time. In WebMediaPlayerMS, I think there is > a simpler solution: just record the last deadline_max passed in > UpdateCurrentFrame, when a new frame comes to OnFrameAvailable (which never > stops even when the tab is invisible), if Now>last recorded deadline_max, then > we call VRA::RemoveExpiredFrames to let old frames go. Well, delivering begin frame callbacks is expensive in terms of power, if you have no frames, you shouldn't be setting this flag. It sounds like you're saying you'll accumulate massive amounts of frames within the VRA, that would take a lot of memory usage, no? Also what are you going to do about WebGL copies? I don't think any solution that doesn't result in you using VideoFrameCompositor is going to work very well.
On 2015/08/04 23:19:14, DaleCurtis wrote: > +sunnyps in case he wants to add anything about StartRendering(). > > https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms.cc (right): > > https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... > content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); > On 2015/08/04 22:13:24, qiangchenC wrote: > > On 2015/08/04 21:49:26, DaleCurtis wrote: > > > Where are you calling StopRendering? How are you handling backgrounding of > the > > > tab? If you turn this path on always, you won't get UpdateCurrentFrame > calls > > > when the layer is invisible. > > > > video_frame_provider_client_->StopUsingProvider() will call StopRendering(). > > > > For background rendering, I found VideoFrameCompositor uses a > > background_rendering_timer_ to do background rendering when UpdateCurrentFrame > > does not get called for a period of time. In WebMediaPlayerMS, I think there > is > > a simpler solution: just record the last deadline_max passed in > > UpdateCurrentFrame, when a new frame comes to OnFrameAvailable (which never > > stops even when the tab is invisible), if Now>last recorded deadline_max, then > > we call VRA::RemoveExpiredFrames to let old frames go. > > Well, delivering begin frame callbacks is expensive in terms of power, if you > have no frames, you shouldn't be setting this flag. > > It sounds like you're saying you'll accumulate massive amounts of frames within > the VRA, that would take a lot of memory usage, no? Also what are you going to > do about WebGL copies? I don't think any solution that doesn't result in you > using VideoFrameCompositor is going to work very well. Yes, asking for BeginFrames will cause cpu wakeups and on some platforms also involves turning on a hardware interrupt for vsync.
https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); On 2015/08/04 23:19:14, DaleCurtis wrote: > On 2015/08/04 22:13:24, qiangchenC wrote: > > On 2015/08/04 21:49:26, DaleCurtis wrote: > > > Where are you calling StopRendering? How are you handling backgrounding of > the > > > tab? If you turn this path on always, you won't get UpdateCurrentFrame > calls > > > when the layer is invisible. > > > > video_frame_provider_client_->StopUsingProvider() will call StopRendering(). > > > > For background rendering, I found VideoFrameCompositor uses a > > background_rendering_timer_ to do background rendering when UpdateCurrentFrame > > does not get called for a period of time. In WebMediaPlayerMS, I think there > is > > a simpler solution: just record the last deadline_max passed in > > UpdateCurrentFrame, when a new frame comes to OnFrameAvailable (which never > > stops even when the tab is invisible), if Now>last recorded deadline_max, then > > we call VRA::RemoveExpiredFrames to let old frames go. > > Well, delivering begin frame callbacks is expensive in terms of power, if you > have no frames, you shouldn't be setting this flag. > > It sounds like you're saying you'll accumulate massive amounts of frames within > the VRA, that would take a lot of memory usage, no? Also what are you going to > do about WebGL copies? I don't think any solution that doesn't result in you > using VideoFrameCompositor is going to work very well. I did not quite understand what is " delivering begin frame callbacks". What I found is that when the tab is invisible, UpdateCurrentFrame will not be called. Did you mean that at this time if we do not call client->StopRendering(), there is a performance impact? > It sounds like you're saying you'll accumulate massive amounts of frames within > the VRA, that would take a lot of memory usage, no? Never, when UpdateCurrentFrame is not called, we are going to call RemoveExpiredFrames in OnFrameAvailable to let frames go.
https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); On 2015/08/04 23:45:45, qiangchenC wrote: > > I did not quite understand what is " delivering begin frame callbacks". What I > found is that when the tab is invisible, UpdateCurrentFrame will not be called. > Did you mean that at this time if we do not call client->StopRendering(), there > is a performance impact? You shouldn't keep rendering enabled when the tab is visible and you have nothing to render as it wastes power. If you know you have nothing new to render (i.e., the video is paused in <video> case) you should call StopRendering() to avoid wasting power. > > > It sounds like you're saying you'll accumulate massive amounts of frames > within > > the VRA, that would take a lot of memory usage, no? > > Never, when UpdateCurrentFrame is not called, we are going to call > RemoveExpiredFrames in OnFrameAvailable to let frames go. It sounds like you're going to implement your timeout callback... for which you'd need to add workarounds for WebGL... for which is exactly what VideoFrameCompositor will handle for you :)
On 2015/08/04 23:45:45, qiangchenC wrote: > https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms.cc (right): > > https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... > content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); > On 2015/08/04 23:19:14, DaleCurtis wrote: > > On 2015/08/04 22:13:24, qiangchenC wrote: > > > On 2015/08/04 21:49:26, DaleCurtis wrote: > > > > Where are you calling StopRendering? How are you handling backgrounding of > > the > > > > tab? If you turn this path on always, you won't get UpdateCurrentFrame > > calls > > > > when the layer is invisible. > > > > > > video_frame_provider_client_->StopUsingProvider() will call StopRendering(). > > > > > > > For background rendering, I found VideoFrameCompositor uses a > > > background_rendering_timer_ to do background rendering when > UpdateCurrentFrame > > > does not get called for a period of time. In WebMediaPlayerMS, I think there > > is > > > a simpler solution: just record the last deadline_max passed in > > > UpdateCurrentFrame, when a new frame comes to OnFrameAvailable (which never > > > stops even when the tab is invisible), if Now>last recorded deadline_max, > then > > > we call VRA::RemoveExpiredFrames to let old frames go. > > > > Well, delivering begin frame callbacks is expensive in terms of power, if you > > have no frames, you shouldn't be setting this flag. > > > > It sounds like you're saying you'll accumulate massive amounts of frames > within > > the VRA, that would take a lot of memory usage, no? Also what are you going to > > do about WebGL copies? I don't think any solution that doesn't result in you > > using VideoFrameCompositor is going to work very well. > > I did not quite understand what is " delivering begin frame callbacks". What I > found is that when the tab is invisible, UpdateCurrentFrame will not be called. > Did you mean that at this time if we do not call client->StopRendering(), there > is a performance impact? > > > It sounds like you're saying you'll accumulate massive amounts of frames > within > > the VRA, that would take a lot of memory usage, no? > > Never, when UpdateCurrentFrame is not called, we are going to call > RemoveExpiredFrames in OnFrameAvailable to let frames go. By the way, I did not find a good way to use VideoFrameCompositor in WebMediaPlayerMS, as VideoFrameCompositor is essentially an implementation of cc::VideoFrameProvider. And WebMediaPlayerMS is also a cc::VideoFrameProvider.
Yes, you'll first need to detangle the VideoFrameProvider dependency, but I don't think that would be too hard.
Yes, you'll first need to detangle the VideoFrameProvider dependency, but I don't think that would be too hard.
Updated my code with solution to pause and background rendering. Dale, after looking into the code more deeply, I found obstructions not using VideoFrameCompositor for RTC. 1. Pause: This is a significant difference between RTC and video playing. For RTC, you cannot really pause the stream, which means you cannot prevent new frames coming. When paused, what you can do is to digest those frames without rendering. In some sense, handling pause is easier for RTC, because you do not need to worry about the timestamp ~ TimeTicks realignment after resuming. In another word, for RTC, pause is kind of a synonym with background rendering. But you are right, we can call client->StopRendering() to save some energy during pause. 2. Background rendering: VideoFrameCompositor use a timeout to detect this. Namely, if in 250ms, UpdateCurrentFrame never gets called, then the background rendering thread will wake up to digest frames. Unfortunately, this algorithm will not work for RTC using hardware decoder. Because we have only 4 output buffers for hardware decoder. While in 250ms, about 8 frames will be generated (assuming 30fps), which is more than enough to use up all output buffers. The consequence is severe, as the decoder may throw exception and stop working. My idea is to check the necessity of RemoveExpiredFrame at the time when a new frame comes which is simple as no extra thread is needed, and will detect background rendering at an early stage. https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/160001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:455: client->StartRendering(); On 2015/08/04 23:49:02, DaleCurtis wrote: > On 2015/08/04 23:45:45, qiangchenC wrote: > > > > I did not quite understand what is " delivering begin frame callbacks". What I > > found is that when the tab is invisible, UpdateCurrentFrame will not be > called. > > Did you mean that at this time if we do not call client->StopRendering(), > there > > is a performance impact? > > You shouldn't keep rendering enabled when the tab is visible and you have > nothing to render as it wastes power. If you know you have nothing new to > render (i.e., the video is paused in <video> case) you should call > StopRendering() to avoid wasting power. > > > > > > It sounds like you're saying you'll accumulate massive amounts of frames > > within > > > the VRA, that would take a lot of memory usage, no? > > > > Never, when UpdateCurrentFrame is not called, we are going to call > > RemoveExpiredFrames in OnFrameAvailable to let frames go. > > It sounds like you're going to implement your timeout callback... for which > you'd need to add workarounds for WebGL... for which is exactly what > VideoFrameCompositor will handle for you :) Let me do another patch to explain this issue.
I see, thanks for the explanation qiangchen@. I forgot that WebRTC is pushing frames into WebMediaPlayerMS at a constant rate, so you never need to handle background rendering or WebGL copies, since the current frame is pushed regardless of pickup. lgtm
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #8 (id:240001) has been deleted
When trying trybots, found that Start/StopRendering should be called in compositor thread. Thus made the change. Can you take a look at patch8?
https://codereview.chromium.org/1265433003/diff/260001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/260001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:136: wait_event_.Wait(); Hmm, this is unfortunate. Typically you'd use CompositorTaskRunner::DeleteSoon() to avoid this wait -- which is what WMPI does with the VideoFrameCompositor object. Seems like it might be better for you to do something similar here, have an internal class which manages running on the compositor thread and uses a lock to clear the state on the render thread. You can then pass that object to the compositor thread for deletion. I.e., just move all your helpers into an inner class and add a lock.
Did some structural change for WebMediaPlayerMS. Move all handlers that should take place on compositor thread to an inner class. https://codereview.chromium.org/1265433003/diff/260001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/260001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:136: wait_event_.Wait(); On 2015/08/13 18:06:53, DaleCurtis wrote: > Hmm, this is unfortunate. Typically you'd use CompositorTaskRunner::DeleteSoon() > to avoid this wait -- which is what WMPI does with the VideoFrameCompositor > object. > > Seems like it might be better for you to do something similar here, have an > internal class which manages running on the compositor thread and uses a lock to > clear the state on the render thread. You can then pass that object to the > compositor thread for deletion. > > I.e., just move all your helpers into an inner class and add a lock. Done.
https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:121: compositor_thread_->DeleteSoon(FROM_HERE, compositor_); I don't think this is quite right, you need to ensure the compositor won't access WMPMS from this point forward, so you need some way to stop future callbacks -- you can't wait for the destructor to run on the compositor thread in that case. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:131: class Compositor : public cc::VideoFrameProvider { Hmm, this is larger than I anticipated; I thought you would just move in the helper functions. I think this is big enough that you should split it out into its own class files with a unittest now. How about calling it MediaStreamCompositor?
Maybe, we could land patch 8 first, and do another CL for code refactoring. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:121: compositor_thread_->DeleteSoon(FROM_HERE, compositor_); On 2015/08/14 17:07:56, DaleCurtis wrote: > I don't think this is quite right, you need to ensure the compositor won't > access WMPMS from this point forward, so you need some way to stop future > callbacks -- you can't wait for the destructor to run on the compositor thread > in that case. Compositor class does not have a pointer to WMPMS, and thus never access WMPMS actively. All communications between WMPMS and Compositor is initiated from WMPMS like that WMPMS pushes a frame into the compositor, or WMPMS reads some metadata of the current frame of the compositor. Thus it is safe that WMPMS gets destroyed first. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:131: class Compositor : public cc::VideoFrameProvider { On 2015/08/14 17:07:56, DaleCurtis wrote: > Hmm, this is larger than I anticipated; I thought you would just move in the > helper functions. I think this is big enough that you should split it out into > its own class files with a unittest now. > > How about calling it MediaStreamCompositor? Essentially this is just a code refactoring. The workflow does not change at all. By code searching, I did not find WMPMS had its unittest (Otherwise I would simply split that unittest into two). Would it benefit to write one? If so, I think it could be better for us to land patch8 first and do another CL for this refactoring.
https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:584: current_frame_used_ = true; Technically you don't know this until PutCurrentFrame() is called. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:591: Extra space. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:627: scoped_refptr<media::VideoFrame> new_frame = Hmm, how is this going to work with GpuMemoryBuffers and the like? https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:637: else Don't use else w/ return. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:655: if (!current_frame_.get()) This is just return current_frame_ ? https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:131: class Compositor : public cc::VideoFrameProvider { On 2015/08/14 18:16:39, qiangchenC wrote: > On 2015/08/14 17:07:56, DaleCurtis wrote: > > Hmm, this is larger than I anticipated; I thought you would just move in the > > helper functions. I think this is big enough that you should split it out into > > its own class files with a unittest now. > > > > How about calling it MediaStreamCompositor? > > Essentially this is just a code refactoring. The workflow does not change at > all. By code searching, I did not find WMPMS had its unittest (Otherwise I would > simply split that unittest into two). Would it benefit to write one? > > If so, I think it could be better for us to land patch8 first and do another CL > for this refactoring. I'm okay with that so long as you're signing up to do so in the near term. If this CL gets reverted though I'm going to feel obliged to tell you I told you so :p https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:133: Compositor(scoped_refptr<base::SingleThreadTaskRunner> compositor_thread); explicit const& https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:137: void EnqueueFrame(scoped_refptr<media::VideoFrame> frame); const& https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:160: static void StartRendering(Compositor* compositor); I don't see why you need these to be static. You can do what VFC does and just post a task to the compositor thread from inside this function, since destruction happens on the same thread, tasks won't be interleaved. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:246: Compositor* compositor_; Should probably still be a scoped_ptr and you can just use .release() in destruction.
Thanks for your comments, and I still have questions. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:584: current_frame_used_ = true; On 2015/08/14 18:31:24, DaleCurtis wrote: > Technically you don't know this until PutCurrentFrame() is called. Did you mean to modify current_frame_used_ in PutCurrentFrame? Or should we apply more complicated logic to consider the case "GetCurrentFrame --> OnFrameAvailable --> PutCurrentFrame"? https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:591: On 2015/08/14 18:31:24, DaleCurtis wrote: > Extra space. Done. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:627: scoped_refptr<media::VideoFrame> new_frame = On 2015/08/14 18:31:24, DaleCurtis wrote: > Hmm, how is this going to work with GpuMemoryBuffers and the like? Hmm, that's kind of out of my scope right now. As I just did a code refactoring here, everything is from existing code. We could find a way to resolve conflicts if any. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:637: else On 2015/08/14 18:31:24, DaleCurtis wrote: > Don't use else w/ return. Done. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:655: if (!current_frame_.get()) On 2015/08/14 18:31:24, DaleCurtis wrote: > This is just return current_frame_ ? If we set current_frame_used_ in PutCurrentFrame, this function can be removed. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:133: Compositor(scoped_refptr<base::SingleThreadTaskRunner> compositor_thread); On 2015/08/14 18:31:25, DaleCurtis wrote: > explicit const& Done. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:137: void EnqueueFrame(scoped_refptr<media::VideoFrame> frame); On 2015/08/14 18:31:25, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:160: static void StartRendering(Compositor* compositor); On 2015/08/14 18:31:25, DaleCurtis wrote: > I don't see why you need these to be static. You can do what VFC does and just > post a task to the compositor thread from inside this function, since > destruction happens on the same thread, tasks won't be interleaved. Not quite familiar with base::Bind. It seems I have to make class T inherit RefCounted<T>, then T* can be used in Bind for non-static functions. But if I do so, the compiler will ask me to put the destructor in protect or private, then there will be an issue for DeleteSoon. What is the correct work around? https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:246: Compositor* compositor_; On 2015/08/14 18:31:25, DaleCurtis wrote: > Should probably still be a scoped_ptr and you can just use .release() in > destruction. Done.
Sorry saw one more issue with the skcanvas video renderer too. https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:584: current_frame_used_ = true; On 2015/08/14 20:09:42, qiangchenC wrote: > On 2015/08/14 18:31:24, DaleCurtis wrote: > > Technically you don't know this until PutCurrentFrame() is called. > > Did you mean to modify current_frame_used_ in PutCurrentFrame? Or should we > apply more complicated logic to consider the case "GetCurrentFrame --> > OnFrameAvailable --> PutCurrentFrame"? Actually I misremembered the issue. It's more that PutCurrentFrame/GetCurrentFrame won't always be called. https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_fr... Either place is fine, I'm not real sure what |current_Frame_used_| is for anymore. If you leave it in PutCurrentFrame, change this code to just return current_frame_; https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:160: static void StartRendering(Compositor* compositor); On 2015/08/14 20:09:42, qiangchenC wrote: > On 2015/08/14 18:31:25, DaleCurtis wrote: > > I don't see why you need these to be static. You can do what VFC does and just > > post a task to the compositor thread from inside this function, since > > destruction happens on the same thread, tasks won't be interleaved. > > Not quite familiar with base::Bind. It seems I have to make class T inherit > RefCounted<T>, then T* can be used in Bind for non-static functions. But if I do > so, the compiler will ask me to put the destructor in protect or private, then > there will be an issue for DeleteSoon. What is the correct work around? You can use base::Unretained(compositor_) here since the lifetime of this class i.e. everything that might call start/stop is tied to the lifetime of WMPMS. By virtue this means any start/stop (and no future start/stop) can be called after the DeleteSoon is passed to the compositor thread. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:216: compositor_.get(), &video_renderer_)); |video_renderer_| needs to live as long as |compositor_| to use it here. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:507: scoped_refptr<base::SingleThreadTaskRunner> const& compositor_thread) No, this is what I meant: const scoped_refptr<base::SingleThreadTaskRunner>&
Fixed https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/280001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:584: current_frame_used_ = true; On 2015/08/14 20:47:28, DaleCurtis wrote: > On 2015/08/14 20:09:42, qiangchenC wrote: > > On 2015/08/14 18:31:24, DaleCurtis wrote: > > > Technically you don't know this until PutCurrentFrame() is called. > > > > Did you mean to modify current_frame_used_ in PutCurrentFrame? Or should we > > apply more complicated logic to consider the case "GetCurrentFrame --> > > OnFrameAvailable --> PutCurrentFrame"? > > Actually I misremembered the issue. It's more that > PutCurrentFrame/GetCurrentFrame won't always be called. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_fr... > > Either place is fine, I'm not real sure what |current_Frame_used_| is for > anymore. If you leave it in PutCurrentFrame, change this code to just return > current_frame_; For WMPMS, it is just used to keep a statistics on how many frames got dropped. If you are not quite sure, let me change the code back. As to put it in PutCurrentFrame is thread unsafe. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:216: compositor_.get(), &video_renderer_)); On 2015/08/14 20:47:29, DaleCurtis wrote: > |video_renderer_| needs to live as long as |compositor_| to use it here. Moved video_renderer_ into Compositor. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:507: scoped_refptr<base::SingleThreadTaskRunner> const& compositor_thread) On 2015/08/14 20:47:29, DaleCurtis wrote: > No, this is what I meant: > const scoped_refptr<base::SingleThreadTaskRunner>& Essentially the same thing here. For C++, const applies to whatever on the immediate left of "const". If const is leftmost, then it applies to its immediate right. Example "const int*" == "int const*" != "int* const". I did code search, and found both occurrences of "T const&" and "const T&". If you prefer one to the other, I can make the change.
Whoops, some of my comments got duplicated since I didn't realize you'd uploaded a new patch set in between. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:98: scoped_refptr<base::SingleThreadTaskRunner> compositor_thread) const& and rename to compositor_task_runner https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:352: auto frame = compositor_->GetCurrentFrame(); Either use auto everywhere you make this call or explicitly name the type. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:419: scoped_refptr<media::VideoFrame> video_frame = compositor_->GetCurrentFrame(); Inconsistent use of auto. https://codereview.chromium.org/1265433003/diff/300001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:507: scoped_refptr<base::SingleThreadTaskRunner> const& compositor_thread) On 2015/08/14 21:53:48, qiangchenC wrote: > On 2015/08/14 20:47:29, DaleCurtis wrote: > > No, this is what I meant: > > const scoped_refptr<base::SingleThreadTaskRunner>& > > Essentially the same thing here. For C++, const applies to whatever on the > immediate left of "const". If const is leftmost, then it applies to its > immediate right. Example "const int*" == "int const*" != "int* const". > > I did code search, and found both occurrences of "T const&" and "const T&". If > you prefer one to the other, I can make the change. There's like 200 const& and >20000 const T& -- please change :) https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:352: auto frame = compositor_->GetCurrentFrame(); Inconistent use of auto. Use scoped_refptr<VideoFrame> here since you do so elsehwere. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:464: bool size_changed = !compositor_->HasCurrentFrame() || No need for check since CurrentFrameSize returns gfx::Size() ? https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:573: return current_frame_; Might need !! to avoid compilation errors on windows, I forget. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:580: if (!current_frame_.get()) Conditional necessary? If current frame is null, does it matter if we set current_frame_used_ to true? You might need a !current_frame and !current_frame_used above. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:630: if (current_frame_.get()) Ternary would be cleaner. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:649: if (!current_frame_.get()) Should just delete this method after switching to counting dropped frames as I've mentioned in my previous comment. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:66: scoped_refptr<base::SingleThreadTaskRunner> compositor_thread); const&, compositor_task_runner ? https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:133: Compositor( explicit https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:140: gfx::Size CurrentFrameSize(); Are these two necessary since you have GetCurrentFrame() ? https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:142: unsigned TotalFrameCount(); Add GetXXX and const modifier to both of these. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:145: // Get a reference to Current Frame. Different from GetCurrentFrame This is pretty messy, can you avoid having both methods? Two methods that return the current frame isn't great. The mixed use of GetXXX and unprefixed methods is also unfortunate. The right way to count dropped frames is instead to set a marker during UpdateCurrentFrame() and clear it during PutCurrentFrame. If the next UpdateCurrentFrame() comes along without the marker being cleared, you can count the frame as dropped. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:179: // |current_frame_used_| is updated on both main and compositing thread. Insert blank line.
https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:352: auto frame = compositor_->GetCurrentFrame(); On 2015/08/17 21:31:29, DaleCurtis wrote: > Inconistent use of auto. Use scoped_refptr<VideoFrame> here since you do so > elsehwere. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:464: bool size_changed = !compositor_->HasCurrentFrame() || On 2015/08/17 21:31:29, DaleCurtis wrote: > No need for check since CurrentFrameSize returns gfx::Size() ? Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:573: return current_frame_; On 2015/08/17 21:31:29, DaleCurtis wrote: > Might need !! to avoid compilation errors on windows, I forget. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:580: if (!current_frame_.get()) On 2015/08/17 21:31:29, DaleCurtis wrote: > Conditional necessary? If current frame is null, does it matter if we set > current_frame_used_ to true? You might need a !current_frame and > !current_frame_used above. N/A https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:630: if (current_frame_.get()) On 2015/08/17 21:31:29, DaleCurtis wrote: > Ternary would be cleaner. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:649: if (!current_frame_.get()) On 2015/08/17 21:31:29, DaleCurtis wrote: > Should just delete this method after switching to counting dropped frames as > I've mentioned in my previous comment. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:66: scoped_refptr<base::SingleThreadTaskRunner> compositor_thread); On 2015/08/17 21:31:30, DaleCurtis wrote: > const&, compositor_task_runner ? Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:133: Compositor( On 2015/08/17 21:31:30, DaleCurtis wrote: > explicit Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:140: gfx::Size CurrentFrameSize(); On 2015/08/17 21:31:30, DaleCurtis wrote: > Are these two necessary since you have GetCurrentFrame() ? N/A now. Add staging_frame_. Just think it as a VRA with only one frame max. Then the code path is very close to what it will be after applying VRA. These two functions are used to return the data about the most recent incoming frame. Where current_frame_ is for compositing use. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:142: unsigned TotalFrameCount(); On 2015/08/17 21:31:30, DaleCurtis wrote: > Add GetXXX and const modifier to both of these. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:145: // Get a reference to Current Frame. Different from GetCurrentFrame On 2015/08/17 21:31:30, DaleCurtis wrote: > This is pretty messy, can you avoid having both methods? Two methods that return > the current frame isn't great. The mixed use of GetXXX and unprefixed methods is > also unfortunate. > > The right way to count dropped frames is instead to set a marker during > UpdateCurrentFrame() and clear it during PutCurrentFrame. If the next > UpdateCurrentFrame() comes along without the marker being cleared, you can count > the frame as dropped. Done. https://codereview.chromium.org/1265433003/diff/320001/content/renderer/media... content/renderer/media/webmediaplayer_ms.h:179: // |current_frame_used_| is updated on both main and compositing thread. On 2015/08/17 21:31:30, DaleCurtis wrote: > Insert blank line. Done.
lgtm, thanks for your patience!
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/1265433003/#ps340001 (title: "Style (Rebranched)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
qiangchen@chromium.org changed reviewers: + rsesek@chromium.org
Robert, can you take a look at this CL, especially render_frame_impl.cc, we just need to pass the compositor_task_runner to WebMediaPlayerMS.
On 2015/08/18 18:22:07, qiangchenC wrote: > Robert, can you take a look at this CL, especially render_frame_impl.cc, we just > need to pass the compositor_task_runner to WebMediaPlayerMS. I'm not really familiar with this code at all. Is there a reason you selected me as a reviewer?
Because you are listed in OWNER of render_frame_impl. Do you know anyone who is suitable for this review? On Tue, Aug 18, 2015 at 12:09 PM, <rsesek@chromium.org> wrote: > On 2015/08/18 18:22:07, qiangchenC wrote: > >> Robert, can you take a look at this CL, especially render_frame_impl.cc, >> we >> > just > >> need to pass the compositor_task_runner to WebMediaPlayerMS. >> > > I'm not really familiar with this code at all. Is there a reason you > selected me > as a reviewer? > > https://codereview.chromium.org/1265433003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
qiangchen@chromium.org changed reviewers: + piman@chromium.org - rsesek@chromium.org
Hi, Antoine Labour:
Can you take a look at the change on render_frame_impl.cc? We've just need
to pass the compositor_task_runner to WebMediaPlayerMS.
Qiang
On 2015/08/18 19:20:53, chromium-reviews wrote: > Because you are listed in OWNER of render_frame_impl. Do you know anyone > who is suitable for this review? I'm not. I'm listed in content/renderer/OWNERS for a specific file.
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, miu@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1265433003/#ps400001 (title: "Initial value issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265433003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265433003/400001
Message was sent while issue was closed.
Committed patchset #15 (id:400001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/dde7ac68de3071061c47a1986a3344da0ffe1f7d Cr-Commit-Position: refs/heads/master@{#344572}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:400001) has been created in https://codereview.chromium.org/1304863002/ by pfeldman@chromium.org. The reason for reverting is: Blink layout test failures: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=med....
Message was sent while issue was closed.
On 2015/08/20 21:48:08, pfeldman wrote: > A revert of this CL (patchset #15 id:400001) has been created in > https://codereview.chromium.org/1304863002/ by mailto:pfeldman@chromium.org. > > The reason for reverting is: Blink layout test failures: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=med.... Is there a trybot that I can do the test? From one stacktrace in your failure link, I doubt the cause could be that I did not consider the case when there isn't a Compositor thread for Video Playing. But I need to make it for sure before recommitting. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
