Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007463005:
Paint first frame faster, don't crash with no frames during EOS. (Closed)
|
Created:
4 years, 7 months ago by DaleCurtis Modified:
4 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaint first frame faster, don't crash with no frames during EOS.
Since we're no longer using the sink to paint the first frame, it's
easier to start painting the first frame as soon as we know it's
"a sure thing." Which is when we (1) have at least two frames
queued, or (2) the first frame has a timestamp >= start timestamp,
or (3) know that no more frames are coming.
As part of this, fixes a nullptr crash caused by deciding if EOS
should be triggered before removing frames for underflow, which
leaves painting with no frames.
Additionally adds a unique identifier to VideoFrames to resolve
incorrect caching issues that the above changes exposed.
BUG=614099
TEST=new unittest
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/046faf603abbd7886bd01bc99078d716145d3680
Cr-Commit-Position: refs/heads/master@{#397468}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comment. Also consider start timestamp. #Patch Set 3 : Remove DCHECK. #
Total comments: 6
Patch Set 4 : Add unique id to VideoFrame. #
Total comments: 7
Patch Set 5 : Remove frame_ptr. #
Total comments: 4
Patch Set 6 : Sanitize PlaneResource. #Patch Set 7 : Add comments. #
Total comments: 6
Patch Set 8 : Comments. #
Total comments: 2
Patch Set 9 : Drop whitespace. #Patch Set 10 : Check ref count instead. #
Total comments: 2
Patch Set 11 : Whoops, fix comment. #
Messages
Total messages: 75 (23 generated)
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
LGTM with nits https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... File media/renderers/video_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl.cc:405: // enough frames to know it's definitely the first frame or (2) there may be Could you please explain more about why when we see two frame we know a frame is definitely the first frame? https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... File media/renderers/video_renderer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl_unittest.cc:219: void WaitForPendingRead() { Should this be renamed to WaitForPendingDecode? https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl_unittest.cc:234: void SatisfyPendingRead() { ditto
Description was changed from ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we either (1) have at least two frames queued, or (2) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. BUG=614099 TEST=new unittest ========== to ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. BUG=614099 TEST=new unittest ==========
The CQ bit was checked by dalecurtis@chromium.org
Thanks for review! https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... File media/renderers/video_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl.cc:405: // enough frames to know it's definitely the first frame or (2) there may be On 2016/05/24 at 21:46:29, xhwang wrote: > Could you please explain more about why when we see two frame we know a frame is definitely the first frame? Done. Also fixed this such that it will also consider the start timestamp. So either we have two frames which straddle the start timestamp or the first frame is >= the start timestamp. https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... File media/renderers/video_renderer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl_unittest.cc:219: void WaitForPendingRead() { On 2016/05/24 at 21:46:29, xhwang wrote: > Should this be renamed to WaitForPendingDecode? Done. https://chromiumcodereview.appspot.com/2007463005/diff/1/media/renderers/vide... media/renderers/video_renderer_impl_unittest.cc:234: void SatisfyPendingRead() { On 2016/05/24 at 21:46:29, xhwang wrote: > ditto Done.
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2007463005/#ps20001 (title: "Comment. Also consider start timestamp.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dalecurtis@chromium.org changed reviewers: + dcash83@gmail.com, hubbe@chromium.org
Hmm, this is a bit scary looking. Looks like the faster painting might be exposing some compositing issues. cc:hubbe who has worked with this code recently and dcastagna who might know what's up. Will take a closer look tomorrow. [1008:1988:0524/184145:6065053:FATAL:video_resource_updater.cc(166)] Check failed: !plane_resource.destructed. ERROR: reused the destructed resource. timestamp = 0.967634s Backtrace: base::debug::StackTrace::StackTrace [0x0000000143A1B2C1+33] logging::LogMessage::~LogMessage [0x00000001439AEFCC+76] cc::VideoResourceUpdater::PlaneResourceMatchesUniqueID [0x0000000140A50615+181] cc::VideoResourceUpdater::CreateForSoftwarePlanes [0x0000000140A4EDB7+935] cc::VideoResourceUpdater::CreateExternalResourcesFromVideoFrame [0x0000000140A4E13B+203] cc::VideoLayerImpl::WillDraw [0x00000001409E047B+459] cc::LayerTreeHostImpl::CalculateRenderPasses [0x0000000140A98326+2486] cc::LayerTreeHostImpl::PrepareToDraw [0x0000000140A9DE60+1776] cc::ProxyImpl::DrawAndSwapInternal [0x0000000140AC778D+301] cc::ProxyImpl::ScheduledActionDrawAndSwapIfPossible [0x0000000140AC9150+336] cc::Scheduler::ProcessScheduledActions [0x0000000140A5B573+723] cc::Scheduler::OnBeginImplFrameDeadline [0x0000000140A5ADF1+273] base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl cc::UniqueNotifier::*)(void) __ptr64>,void __cdecl(cc::UniqueNotifier * __ptr64),base::WeakPtr<cc::UniqueNotifier> >,base::internal::Inv [0x0000000140A526F4+84] base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl test_runner::TestRunner::WorkQueue::*)(void) __ptr64>,void __cdecl(test_runner::TestRunner::WorkQueue * __ptr64),base::WeakPtr<test_runn [0x00000001439162F4+84] base::debug::TaskAnnotator::RunTask [0x0000000143A1F79C+316] base::MessageLoop::RunTask [0x00000001439D1C9C+876] base::MessageLoop::DoWork [0x00000001439D0B70+752] base::MessagePumpDefault::Run [0x0000000143A61758+296] base::RunLoop::Run [0x00000001439CD72E+46] base::MessageLoop::Run [0x00000001439D183B+107] base::Thread::ThreadMain [0x0000000143A1465D+557] base::PlatformThread::Sleep [0x00000001439CE918+424] BaseThreadInitThunk [0x00000000771159CD+13] RtlUserThreadStart [0x000000007734B891+33] [1008:1988:0524/184145:6065162:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was called from a thread that disallows IO! If this thread really should be allowed to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup. Backtrace: base::debug::StackTrace::StackTrace [0x0000000143A1B2C1+33] logging::LogMessage::~LogMessage [0x00000001439AEFCC+76] base::ThreadRestrictions::AssertIOAllowed [0x0000000143A1C004+180] base::GetCurrentDirectoryW [0x00000001439C15B3+51] logging::LogMessage::Init [0x00000001439AFEDD+1101] logging::LogMessage::~LogMessage [0x00000001439AF0D8+344] cc::VideoResourceUpdater::PlaneResourceMatchesUniqueID [0x0000000140A50615+181] cc::VideoResourceUpdater::CreateForSoftwarePlanes [0x0000000140A4EDB7+935] cc::VideoResourceUpdater::CreateExternalResourcesFromVideoFrame [0x0000000140A4E13B+203] cc::VideoLayerImpl::WillDraw [0x00000001409E047B+459] cc::LayerTreeHostImpl::CalculateRenderPasses [0x0000000140A98326+2486] cc::LayerTreeHostImpl::PrepareToDraw [0x0000000140A9DE60+1776] cc::ProxyImpl::DrawAndSwapInternal [0x0000000140AC778D+301] cc::ProxyImpl::ScheduledActionDrawAndSwapIfPossible [0x0000000140AC9150+336] cc::Scheduler::ProcessScheduledActions [0x0000000140A5B573+723] cc::Scheduler::OnBeginImplFrameDeadline [0x0000000140A5ADF1+273] base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl cc::UniqueNotifier::*)(void) __ptr64>,void __cdecl(cc::UniqueNotifier * __ptr64),base::WeakPtr<cc::UniqueNotifier> >,base::internal::Inv [0x0000000140A526F4+84] base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl test_runner::TestRunner::WorkQueue::*)(void) __ptr64>,void __cdecl(test_runner::TestRunner::WorkQueue * __ptr64),base::WeakPtr<test_runn [0x00000001439162F4+84] base::debug::TaskAnnotator::RunTask [0x0000000143A1F79C+316] base::MessageLoop::RunTask [0x00000001439D1C9C+876] base::MessageLoop::DoWork [0x00000001439D0B70+752] base::MessagePumpDefault::Run [0x0000000143A61758+296] base::RunLoop::Run [0x00000001439CD72E+46] base::MessageLoop::Run [0x00000001439D183B+107] base::Thread::ThreadMain [0x0000000143A1465D+557] base::PlatformThread::Sleep [0x00000001439CE918+424] BaseThreadInitThunk [0x00000000771159CD+13] RtlUserThreadStart [0x000000007734B891+33]
dalecurtis@chromium.org changed reviewers: + dcastagna@chromium.org - dcash83@gmail.com
Whoops actually cc dcastagna this time.
On 2016/05/25 at 16:03:44, dalecurtis wrote: > Whoops actually cc dcastagna this time. When a software VideoFrame is sent to the compositor, VideoResourceUpdater caches GL resources for that VideoFrame. The key for the cache is the raw pointer of the video frame. It seems like a VideoFrame raw pointer matches a previous one while the previous VideoFrame already called its destruction callback (that happens in the dtor.) Is it possible that a new VideoFrame is created with the same address of a previous one that has been destroyed? ccing xjz@ that added the check to make sure this wouldn't happen.
On 2016/05/25 16:22:26, Daniele Castagna wrote: > On 2016/05/25 at 16:03:44, dalecurtis wrote: > > Whoops actually cc dcastagna this time. > > When a software VideoFrame is sent to the compositor, VideoResourceUpdater > caches GL resources for that VideoFrame. > The key for the cache is the raw pointer of the video frame. It seems like a > VideoFrame raw pointer matches a previous one while the previous VideoFrame > already called its destruction callback (that happens in the dtor.) > > Is it possible that a new VideoFrame is created with the same address of a > previous one that has been destroyed? > > ccing xjz@ that added the check to make sure this wouldn't happen. That check was added to prevent the reusing of destructed VideoFrame in the cache in the case that the VideoFrame timestamp is not set or not correctly set. It shouldn't be hit if the timestamp is set correctly (i.e., unique for each VideoFrame). The CL is here: https://codereview.chromium.org/1688033005/
Given that this is only happening with a theora video, it's likely the timestamps are suspect. I'll get a windows build going and take a look at what's going on.
On 2016/05/25 at 20:00:20, dalecurtis wrote: > Given that this is only happening with a theora video, it's likely the timestamps are suspect. I'll get a windows build going and take a look at what's going on. You might be able to reproduce the issue on linux/mac disabling GMB videoframes with --disable-gpu-memory-buffer-video-frames.
No luck on Linux w/o GMB, checking Windows.
Hmm can't repro on Windows locally with ia32 or x64. Will retry failures on bot and see if this is an unrelated issue...
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/20001
Passed this time, but I think the media needs to be reencoded, the last 6 frames are repeated: [26172:23724:0526/145424:176525484:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 0 ... [26172:23724:0526/145425:176526468:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 800.801 [26172:23724:0526/145425:176526500:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 834.168 [26172:23724:0526/145425:176526531:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 867.534 [26172:23724:0526/145425:176526546:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 900.901 [26172:23724:0526/145425:176526593:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 934.268 [26172:23724:0526/145425:176526656:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 967.634 [26172:23724:0526/145425:176526796:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 800.801 [26172:23724:0526/145425:176526812:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 834.168 [26172:23724:0526/145425:176526812:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 867.534 [26172:23724:0526/145425:176526828:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 900.901 [26172:23724:0526/145425:176526843:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 934.268 [26172:23724:0526/145425:176526843:ERROR:video_renderer_impl.cc(385)] media::VideoRendererImpl::FrameReady : 967.634
dalecurtis@chromium.org changed reviewers: + xjz@chromium.org
Ah actually that's expected since the test plays until end and then seeks back a little bit, so it replays a few frames. I'm guessing every once in a while the seek and allocation pattern align such that it looks like we've used a destructed resource?
On 2016/05/26 at 23:45:43, dalecurtis wrote: > Ah actually that's expected since the test plays until end and then seeks back a little bit, so it replays a few frames. I'm guessing every once in a while the seek and allocation pattern align such that it looks like we've used a destructed resource? I'd say the assumption made in the compositor that two videoframes with same raw_ptr and same timestamps at different times are the very same it's wrong?
On 2016/05/26 at 23:49:18, dcastagna wrote: > On 2016/05/26 at 23:45:43, dalecurtis wrote: > > Ah actually that's expected since the test plays until end and then seeks back a little bit, so it replays a few frames. I'm guessing every once in a while the seek and allocation pattern align such that it looks like we've used a destructed resource? > > I'd say the assumption made in the compositor that two videoframes with same raw_ptr and same timestamps at different times are the very same it's wrong? Yeah, or at least it should be invalidated after a seek. Let me see if I can figure out a way to tweak the check.
Description was changed from ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. BUG=614099 TEST=new unittest ========== to ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. BUG=614099 TEST=new unittest CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
dalecurtis@chromium.org changed reviewers: + danakj@chromium.org
Hmm, there's no easy way to invalidate the cache from WMPI short of some gnarly hacks; so instead I'm just going to change the DCHECK to a DLOG(WARNING). +danakj for cc/ review.
https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:166: // This can't be a DCHECK() because there are cases where frames may be What's the point of a warning if it can happen? In this case the VideoFrame was destroyed. The texture wasn't?
https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:169: // few milliseconds into the past can trip this check. In this case, is it possible that the regenerated frame is different with the old one that having same raw_ptr? If yes, the texture will be incorrectly re-used.
https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:166: // This can't be a DCHECK() because there are cases where frames may be On 2016/05/27 at 00:12:10, danakj_OOO_back_5.31 wrote: > What's the point of a warning if it can happen? > > In this case the VideoFrame was destroyed. The texture wasn't? Apparently. The VideoFrame was destroyed, recreated and happened to end up with the same frame pointer. I don't exactly understand what governs software frame lifetimes. The layer is certainly still alive during this process, which seems to hold the VideoResourceUpdater and thus these cached instances. https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:169: // few milliseconds into the past can trip this check. On 2016/05/27 at 00:19:44, xjz wrote: > In this case, is it possible that the regenerated frame is different with the old one that having same raw_ptr? If yes, the texture will be incorrectly re-used. It's highly unlikely, but possible. Consider a generated MSE stream or similar where timestamps are a product of the appended data modulo some JS attributes.
https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:169: // few milliseconds into the past can trip this check. On 2016/05/27 02:39:12, DaleCurtis wrote: > On 2016/05/27 at 00:19:44, xjz wrote: > > In this case, is it possible that the regenerated frame is different with the > old one that having same raw_ptr? If yes, the texture will be incorrectly > re-used. > > It's highly unlikely, but possible. Consider a generated MSE stream or similar > where timestamps are a product of the appended data modulo some JS attributes. This brings up this issue again: how does VideoResourceUpdater know that the cached resource could be reused? Maybe we still need add some kind of ID that is unique to each created VideoFrame?
PTAL. Switched to using a real unique (per-process) identifier. https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:169: // few milliseconds into the past can trip this check. On 2016/05/27 at 16:08:40, xjz wrote: > On 2016/05/27 02:39:12, DaleCurtis wrote: > > On 2016/05/27 at 00:19:44, xjz wrote: > > > In this case, is it possible that the regenerated frame is different with the > > old one that having same raw_ptr? If yes, the texture will be incorrectly > > re-used. > > > > It's highly unlikely, but possible. Consider a generated MSE stream or similar > > where timestamps are a product of the appended data modulo some JS attributes. > > This brings up this issue again: how does VideoResourceUpdater know that the cached resource could be reused? Maybe we still need add some kind of ID that is unique to each created VideoFrame? Yes, the cache is not correct sometimes, I realized there's an easy way to fix this. Added a unique identifier to each created VideoFrame (wrapped or otherwise).
https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:177: plane_resource->frame_ptr = video_frame; Is frame_ptr still necessary anywhere else?
https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:177: plane_resource->frame_ptr = video_frame; On 2016/05/27 at 20:53:53, Daniele Castagna wrote: > Is frame_ptr still necessary anywhere else? Nope! Removed.
This LGTM. nits: https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.h:97: // These last three members will be used for identifying the data stored in s/three/two https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.h:101: const void* frame_ptr; Seems that this pointer can be removed now if we trust the |unique_frame_id|. Or to be safer, we can use both to identify the VideoFrame. So either remove this |frame_ptr| or add |video_frame| back to |PlaneResourceMatchesUniqueId()| input parameters? https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.h:106: // same unique identifier (which should not be possible). Maybe we can remove this DCHECK now?
https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:150: unique_frame_id(0), Can we have a valid |VideoFrame| with 0 as |unique_frame_id|? We may need an invalid id here to indicate that the resource is not valid yet. Or maybe add the |videoFrame| ptr back so that we know it is invalid if the ptr is null? https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:174: const media::VideoFrame* video_frame, nit: Maybe make this and |PlaneResourceMatchesUniqueID()| consistent? Either both using |video_frame| or using |unique_frame_id| as input parameters?
Description was changed from ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. BUG=614099 TEST=new unittest CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. Additionally adds a unique identifier to VideoFrames to resolve incorrect caching issues that the above changes exposed. BUG=614099 TEST=new unittest CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
All done. Given the fragility of these resource identifiers I opted to convert PlaneResource to a class with some private members and add some DCHECKs. https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.h:101: const void* frame_ptr; On 2016/05/27 at 21:04:32, xjz wrote: > Seems that this pointer can be removed now if we trust the |unique_frame_id|. Or to be safer, we can use both to identify the VideoFrame. So either remove this |frame_ptr| or add |video_frame| back to |PlaneResourceMatchesUniqueId()| input parameters? Already done in PS#3. https://codereview.chromium.org/2007463005/diff/60001/cc/resources/video_reso... cc/resources/video_resource_updater.h:106: // same unique identifier (which should not be possible). On 2016/05/27 at 21:04:32, xjz wrote: > Maybe we can remove this DCHECK now? I thought the same thing, so since you do too I'll go ahead and remove it. https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:150: unique_frame_id(0), On 2016/05/27 at 21:21:53, xjz wrote: > Can we have a valid |VideoFrame| with 0 as |unique_frame_id|? We may need an invalid id here to indicate that the resource is not valid yet. Or maybe add the |videoFrame| ptr back so that we know it is invalid if the ptr is null? Added a bool to track this instead of the frame ptr and made these methods a bit safer. https://codereview.chromium.org/2007463005/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:174: const media::VideoFrame* video_frame, On 2016/05/27 at 21:21:53, xjz wrote: > nit: Maybe make this and |PlaneResourceMatchesUniqueID()| consistent? Either both using |video_frame| or using |unique_frame_id| as input parameters? Done.
LGTM. Thanks!
https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:97: // Returns true if this resource matches the unique identifiers of a another "of a another" https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: const unsigned resource_id; These should be named with a trailing underscore if this is a class. https://google.github.io/styleguide/cppguide.html#Variable_Names And should become private then (sorry). https://google.github.io/styleguide/cppguide.html#Access_Control
I like the changes tho.
https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: const unsigned resource_id; On 2016/05/31 at 20:35:58, danakj wrote: > These should be named with a trailing underscore if this is a class. https://google.github.io/styleguide/cppguide.html#Variable_Names > > And should become private then (sorry). https://google.github.io/styleguide/cppguide.html#Access_Control You caught me trying to escape :) This is mostly easy except for ref_count(). What do you want do do with that member? Would you prefer set_ref_count()+get_ref_count(), is_zero()+set_zero()+increment()+decrement(), or a non-const&? Note, we need a zero check, zero set, increment and decrement operations in this case.
https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: const unsigned resource_id; On 2016/06/01 20:46:48, DaleCurtis wrote: > On 2016/05/31 at 20:35:58, danakj wrote: > > These should be named with a trailing underscore if this is a class. > https://google.github.io/styleguide/cppguide.html#Variable_Names > > > > And should become private then (sorry). > https://google.github.io/styleguide/cppguide.html#Access_Control > > You caught me trying to escape :) This is mostly easy except for ref_count(). > What do you want do do with that member? Would you prefer > set_ref_count()+get_ref_count(), is_zero()+set_zero()+increment()+decrement(), > or a non-const&? Note, we need a zero check, zero set, increment and decrement > operations in this case. 4 operations sounds fine. Maybe name them including the fact they are refs. HasAnyRefs() DropAllRefs() IncrementRefs() DecrementRefs() or the like?
Patchset #8 (id:140001) has been deleted
All cleaned up. https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:97: // Returns true if this resource matches the unique identifiers of a another On 2016/05/31 at 20:35:58, danakj wrote: > "of a another" Done. https://codereview.chromium.org/2007463005/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: const unsigned resource_id; On 2016/06/01 at 21:02:02, danakj wrote: > On 2016/06/01 20:46:48, DaleCurtis wrote: > > On 2016/05/31 at 20:35:58, danakj wrote: > > > These should be named with a trailing underscore if this is a class. > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > > > > > And should become private then (sorry). > > https://google.github.io/styleguide/cppguide.html#Access_Control > > > > You caught me trying to escape :) This is mostly easy except for ref_count(). > > What do you want do do with that member? Would you prefer > > set_ref_count()+get_ref_count(), is_zero()+set_zero()+increment()+decrement(), > > or a non-const&? Note, we need a zero check, zero set, increment and decrement > > operations in this case. > > 4 operations sounds fine. Maybe name them including the fact they are refs. > > HasAnyRefs() > DropAllRefs() > IncrementRefs() > DecrementRefs() > > or the like? Done, but with hacker_style().
LGTM https://codereview.chromium.org/2007463005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.h:125: nit: drop this whitespace to discourage people adding stuff in between
The CQ bit was checked by dalecurtis@chromium.org
Thanks for review! https://codereview.chromium.org/2007463005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.h:125: On 2016/06/01 at 21:46:55, danakj wrote: > nit: drop this whitespace to discourage people adding stuff in between Done.
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, xjz@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2007463005/#ps180001 (title: "Drop whitespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/180001
Hmm, something is calling SetUniqueId twice, looking.
Ah, looks like that can happen when a resource of the same size is reused. Perhaps the check should be !has_refs || !has_unique_id
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On Wed, Jun 1, 2016 at 4:34 PM, <dalecurtis@chromium.org> wrote: > Ah, looks like that can happen when a resource of the same size is reused. > Perhaps the check should be !has_refs || !has_unique_id > sgtm > > https://codereview.chromium.org/2007463005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Actually that doesn't work either, the best solution to keep the check I can come up with is only allowing SetUniqueId() to be called when the ref_count is 1, which is a odd private detail. The main trouble is that finding resources is a multistep process that may fail and have to be undone. danakj: Let me know if you'd rather just kill the check. The latest version uses the ref count check.
LGTM https://codereview.chromium.org/2007463005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.h:102: // there is a single reference to the resource (i.e. |ref_count_| == 0). you mean 1?
The CQ bit was checked by dalecurtis@chromium.org
https://codereview.chromium.org/2007463005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2007463005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.h:102: // there is a single reference to the resource (i.e. |ref_count_| == 0). On 2016/06/02 at 00:31:59, danakj wrote: > you mean 1? Whoops, yup.
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, xjz@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2007463005/#ps220001 (title: "Whoops, fix comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/220001
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_...) linux_chromium_chromeos_ozone_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 dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/220001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007463005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007463005/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. Additionally adds a unique identifier to VideoFrames to resolve incorrect caching issues that the above changes exposed. BUG=614099 TEST=new unittest CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Paint first frame faster, don't crash with no frames during EOS. Since we're no longer using the sink to paint the first frame, it's easier to start painting the first frame as soon as we know it's "a sure thing." Which is when we (1) have at least two frames queued, or (2) the first frame has a timestamp >= start timestamp, or (3) know that no more frames are coming. As part of this, fixes a nullptr crash caused by deciding if EOS should be triggered before removing frames for underflow, which leaves painting with no frames. Additionally adds a unique identifier to VideoFrames to resolve incorrect caching issues that the above changes exposed. BUG=614099 TEST=new unittest CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/046faf603abbd7886bd01bc99078d716145d3680 Cr-Commit-Position: refs/heads/master@{#397468} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/046faf603abbd7886bd01bc99078d716145d3680 Cr-Commit-Position: refs/heads/master@{#397468} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
