Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
765533005:
Move the DXVA decoder off the GPU thread. (Closed)
|
Created:
6 years ago by ananta Modified:
6 years ago Reviewers:
DaleCurtis CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, prabhur1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMove the DXVA decoder off the GPU thread.
The DXVA decoder runs on the GPU thread today. When the decoder has enough data to produce an output frame
the IMFTransform::ProcessOutput call blocks while waiting for the media foundation threads to produce the output
frame. This causes lots of jankiness and dropped frames while playing H.264 videos especially HD 1080P videos.
The approach we are taking to resolve this is to move the actual IMFTransform decoding off the GPU thread. This is
implemented by creating a worker thread per DXVA decoder instance. The lifetime of this thread is dependent on the
lifetime of the DXVAVideoDecodeAccelerator class. The IMFTransform decoder instance is created and destroyed on the main
thread. However the calls to ProcessInput/ProcessOutput etc happen on the decoder thread. The decoder state is modified
on both the GPU thread and the worker thread. The methods to get and set the state are now thread safe via InterLocked API
calls.
The copying of the decoded texture to the GPU texture is still done on the main thread. Will work on moving this to the
worker thread in a future patch.
BUG=426675, 430375, 426707
Committed: https://crrev.com/1dd7d968e3f77ec0c90a80c944cd1ec4da8aab11
Cr-Commit-Position: refs/heads/master@{#306929}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased to tip #Patch Set 3 : Leaving H264PROFILE_HIGH profiles as disabled for H/W decode for now #Patch Set 4 : Fixed presubmit #
Total comments: 9
Patch Set 5 : Code review comments #
Total comments: 25
Patch Set 6 : Fixed flush #
Total comments: 1
Patch Set 7 : Removed include #Patch Set 8 : Fixed alignment #
Total comments: 16
Patch Set 9 : Fix flush #
Total comments: 4
Patch Set 10 : Code review comments #
Messages
Total messages: 16 (4 generated)
ananta@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/765533005/diff/1/content/common/gpu/media/dxv... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/1/content/common/gpu/media/dxv... content/common/gpu/media/dxva_video_decode_accelerator.cc:1206: state == kFlushing), We need to allow kFlushing here and below because we can get called for decoding packets in that state.
Thanks for working on this! However, this doesn't seem thread safe. Both threads can set and get the state, so it's possible that each may try to set a different state after the exclusive sections exit... Am I missing something? Instead it seems like you need a thread-safe / locked data structure which the decoder thread will simply pull workload items from. State would be entirely controlled via the main gpu thread. Workloads can be invalidated by clearing the structure. Pending work items could be cancelled via a weakptr or cancellable closure handed to the decoder thread which is posted back to the gpu thread for the final copy. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (left): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1244: DCHECK(CalledOnValidThread()); Can you replace this with the appropriate DCHECK(thread->TaskRunner()) ? Ditto for all the rest. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1218: if (decoder_thread_.thread_id() != ::GetCurrentThreadId()) { decoder_thread_task_runner_->BelongsToCurrentThread() ? https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1395: ::InterlockedCompareExchange(reinterpret_cast<long*>(&state_), This can fail if it's racing against another SetState call. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.h:74: PendingInputs pending_input_buffers_; This should be declared below and the typedef with the Notify method. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.h:167: // the decoder thread. Threadsafe. "thread-safe" or "thread safe" -- up to you.
I changed the SetState call to change the state on the main thread only. Other changes include the following:- 1. Reset(). We stop and start the decoder thread here.Deleted the ResetHelper function from the previous patch. 2. Added a lock to protect the pending_output_samples_ vector. We don't need to protect the pending_input_buffers_ list as it is mostly accessed from the decoder thread and on the main thread after the decoder thread is stopped. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (left): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1244: DCHECK(CalledOnValidThread()); On 2014/12/03 01:04:33, DaleCurtis wrote: > Can you replace this with the appropriate DCHECK(thread->TaskRunner()) ? Ditto > for all the rest. Done. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1218: if (decoder_thread_.thread_id() != ::GetCurrentThreadId()) { On 2014/12/03 01:04:33, DaleCurtis wrote: > decoder_thread_task_runner_->BelongsToCurrentThread() ? Done. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1395: ::InterlockedCompareExchange(reinterpret_cast<long*>(&state_), On 2014/12/03 01:04:33, DaleCurtis wrote: > This can fail if it's racing against another SetState call. SetState now runs on the main thread only. https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): https://codereview.chromium.org/765533005/diff/60001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.h:167: // the decoder thread. Threadsafe. On 2014/12/03 01:04:33, DaleCurtis wrote: > "thread-safe" or "thread safe" -- up to you. Reworded
I'm not convinced this is thread safe yet; there still seem to be a lot of cross-thread accesses to shared data structures. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:504: DCHECK(CalledOnValidThread()); I'd remove base::NonThreadSafe now from inheritance and instead use a DCHECK(main_thread_task_runner) in subsequent calls. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:572: DCHECK_EQ(main_thread_task_runner_.get(), Just DCHECK(main_thread_task_runner_->BelongsToCurrentThread()) ? Ditto for all other instances. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:590: decoder_thread_task_runner_->PostTask(FROM_HERE, Wrapping is wrong, clang-format? https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:622: if (state == kFlushing && !OutputSamplesPresent()) This is racy; ProcessOutputSamples() might be blocked trying to update the output queue when you get here. I think you should always run FlushInternal(), perhaps post it directly to the decoder thread from here for clarity. That method can then exit early if there's nothing to do. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:658: if (state == kFlushing && !OutputSamplesPresent()) Ditto. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:677: if (OutputSamplesPresent()) Ditto. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:684: DVLOG(1) << "DXVAVideoDecodeAccelerator::Reset"; Seems you can add a DCHECK(main_thread_task_runner) here now? https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:690: decoder_thread_.Stop(); Why does the thread need to be stopped? It seems you could just set the reset state (which other methods should early exit from), then trampoline over to the decoder thread for cleanup. The NotifyResetDone should still be posted to the main thread of course. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:701: base::MessageLoop::current()->PostTask( Can you replace all of the MessageLoop::current() with main_thread_task_runner_? https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:704: weak_this_factory_.GetWeakPtr())); Now that you've got the WeakPtr bouncing between threads, you need to make sure your main thread always holds a reference to avoid raciness in WeakPtr construction... see https://codereview.chromium.org/735143002/ for more details on why. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:953: DCHECK_EQ(decoder_thread_task_runner_.get(), Use BelongsToCurrentThread() instead. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1011: bool DXVAVideoDecodeAccelerator::ProcessOutputSample(IMFSample* sample) { I think this whole method needs to be run on the main thread instead; or you need to move ProcessPendingSamples() over in this CL too. output_picture_buffers_.size() doesn't seem thread safe. The lock seems dubious at best too, as noted above with FlushInternal(). Note, if you make this change you can probably keep FlushInternal on the main thread (I think). https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.h:169: // the decoder thread. Threadsafe. Thread safe?
https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:572: DCHECK_EQ(main_thread_task_runner_.get(), On 2014/12/03 22:34:41, DaleCurtis wrote: > Just DCHECK(main_thread_task_runner_->BelongsToCurrentThread()) ? Ditto for all > other instances. Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:590: decoder_thread_task_runner_->PostTask(FROM_HERE, On 2014/12/03 22:34:41, DaleCurtis wrote: > Wrapping is wrong, clang-format? Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:622: if (state == kFlushing && !OutputSamplesPresent()) On 2014/12/03 22:34:41, DaleCurtis wrote: > This is racy; ProcessOutputSamples() might be blocked trying to update the > output queue when you get here. I think you should always run FlushInternal(), > perhaps post it directly to the decoder thread from here for clarity. That > method can then exit early if there's nothing to do. Moved the check for output samples in the decoder thread version of FlushInternal. This is because we only allow one output frame to be present at any given time. I added a call to post the FlushInternal task in ProcessPendingSamples if we are in the kFlushingState https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:658: if (state == kFlushing && !OutputSamplesPresent()) On 2014/12/03 22:34:41, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:677: if (OutputSamplesPresent()) On 2014/12/03 22:34:41, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:684: DVLOG(1) << "DXVAVideoDecodeAccelerator::Reset"; On 2014/12/03 22:34:41, DaleCurtis wrote: > Seems you can add a DCHECK(main_thread_task_runner) here now? Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:690: decoder_thread_.Stop(); On 2014/12/03 22:34:41, DaleCurtis wrote: > Why does the thread need to be stopped? It seems you could just set the reset > state (which other methods should early exit from), then trampoline over to the > decoder thread for cleanup. The NotifyResetDone should still be posted to the > main thread of course. Stopping and restarting the decoder thread gets rid of the potential race conditions during reset, like simultaneous access to output samples, etc. Reset is like getting the decoder to a clean state. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:701: base::MessageLoop::current()->PostTask( On 2014/12/03 22:34:42, DaleCurtis wrote: > Can you replace all of the MessageLoop::current() with main_thread_task_runner_? Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:704: weak_this_factory_.GetWeakPtr())); On 2014/12/03 22:34:41, DaleCurtis wrote: > Now that you've got the WeakPtr bouncing between threads, you need to make sure > your main thread always holds a reference to avoid raciness in WeakPtr > construction... see https://codereview.chromium.org/735143002/ for more details > on why. That is a valid concern. However given that the decoder thread is bound with the lifetime of the DXVADecoder class, we should not have a case where the thread outlives the decoder instance. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:953: DCHECK_EQ(decoder_thread_task_runner_.get(), On 2014/12/03 22:34:41, DaleCurtis wrote: > Use BelongsToCurrentThread() instead. Done. https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:1011: bool DXVAVideoDecodeAccelerator::ProcessOutputSample(IMFSample* sample) { On 2014/12/03 22:34:41, DaleCurtis wrote: > I think this whole method needs to be run on the main thread instead; or you > need to move ProcessPendingSamples() over in this CL too. > output_picture_buffers_.size() doesn't seem thread safe. The lock seems dubious > at best too, as noted above with FlushInternal(). Note, if you make this change > you can probably keep FlushInternal on the main thread (I think). I removed the output_buffers.size check and just posted the ProcessPendingSamples task to the main thread. That funcion checks for the output buffers validity before continuing. We cannot run FlushInternal on the main thread as we call DoDecode there which needs to run on the decoder thread. I think for now we can leave the ProcessOutputSamples on the decoder thread and revisit in the next patch where I was planning to move the copying of the decoded surface to the decoder thread. That is a little tricky due to the GL contexts, etc. https://codereview.chromium.org/765533005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/100001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1173: if (client_ && GetState() == kStopped) Safety check to prevent multiple notifications going out.
https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/80001/content/common/gpu/media... content/common/gpu/media/dxva_video_decode_accelerator.cc:704: weak_this_factory_.GetWeakPtr())); On 2014/12/04 00:45:24, ananta wrote: > On 2014/12/03 22:34:41, DaleCurtis wrote: > > Now that you've got the WeakPtr bouncing between threads, you need to make > sure > > your main thread always holds a reference to avoid raciness in WeakPtr > > construction... see https://codereview.chromium.org/735143002/ for more > details > > on why. > > That is a valid concern. However given that the decoder thread is bound with the > lifetime of the DXVADecoder class, we should not have a case where the thread > outlives the decoder instance. It's not necessarily a lifetime issue, it's a raciness issue with the construction of the WeakReference. See the non-atomicity of the GetRef() method (HasRef() => new WeakRef()). Reference ownership may oscillate as WeakPtr instances come and go across the two threads. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1021: decoder_lock_.Acquire(); It's generally preferred to use an AutoLock here with scoped {} https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1065: base::AutoLock lock(decoder_lock_); Notably since this section may take a while to run, you shouldn't hold the lock except when checking if pending_output_samples is empty and when running pop_front(). https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1119: decoder_thread_task_runner_->PostTask( Seems you can just early return after this PostTask? FlushInternal will run DecodePendingInputBuffers() if necessary. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1222: if (pending_input_buffers_.empty() || OutputSamplesPresent()) Is it necessary to check OutputSamplesPresent() here? Won't it just end up queuing a few more input buffers for decoding which will end up blocked until picture buffers are available? It seems if you remove this restriction you could remove OutputSamplesPresent() everywhere. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1251: if (!decoder_thread_task_runner_->BelongsToCurrentThread()) { There's only a couple callers of this, so it'd be more readable to avoid the trampoline and have the callers explicitly PostTask. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1343: if (!OutputSamplesPresent()) { Why not just always call this? GpuVideoDecoder itself will rate limit, no? https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1445: ::InterlockedCompareExchange(reinterpret_cast<long*>(&state_), Formatting is wrong. Also you should at least DCHECK() that this succeeds.
https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1021: decoder_lock_.Acquire(); On 2014/12/04 01:54:27, DaleCurtis wrote: > It's generally preferred to use an AutoLock here with scoped {} Done. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1065: base::AutoLock lock(decoder_lock_); On 2014/12/04 01:54:27, DaleCurtis wrote: > Notably since this section may take a while to run, you shouldn't hold the lock > except when checking if pending_output_samples is empty and when running > pop_front(). Done. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1119: decoder_thread_task_runner_->PostTask( On 2014/12/04 01:54:27, DaleCurtis wrote: > Seems you can just early return after this PostTask? FlushInternal will run > DecodePendingInputBuffers() if necessary. The state transitions in Flush are a touch complicated at the moment. Returning early here causes problems in Flush completing early leading to issues with hangouts not working correctly. I will look into this in the next patch. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1222: if (pending_input_buffers_.empty() || OutputSamplesPresent()) On 2014/12/04 01:54:27, DaleCurtis wrote: > Is it necessary to check OutputSamplesPresent() here? Won't it just end up > queuing a few more input buffers for decoding which will end up blocked until > picture buffers are available? It seems if you remove this restriction you could > remove OutputSamplesPresent() everywhere. Yes. The Media foundation decoder has a pool of surfaces it uses for decoding. If we exhaust that pool it overwrites the frames. We impose a restriction that we can have only one pending output frame at any given point. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1251: if (!decoder_thread_task_runner_->BelongsToCurrentThread()) { On 2014/12/04 01:54:27, DaleCurtis wrote: > There's only a couple callers of this, so it'd be more readable to avoid the > trampoline and have the callers explicitly PostTask. Done. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1343: if (!OutputSamplesPresent()) { On 2014/12/04 01:54:27, DaleCurtis wrote: > Why not just always call this? GpuVideoDecoder itself will rate limit, no? Done. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1445: ::InterlockedCompareExchange(reinterpret_cast<long*>(&state_), On 2014/12/04 01:54:27, DaleCurtis wrote: > Formatting is wrong. Also you should at least DCHECK() that this succeeds. Done.
lgtm % q cc:prabhur as an FYI to coordinate QA for testing hardware H264 decoding on windows in the next QA pass. https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1236: // If we are scheduled during a flush operation then mark the flush as Does the bug this resolved no longer happen? https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1441: DCHECK(state_ == new_state); DCHECK_EQ() https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.h:246: // WeakPtrFactory for posting tasks back to |this|. move the WeakPtr members to the end of the variable definitions, they should always be destructed first except in rare cases.
https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/140001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1236: // If we are scheduled during a flush operation then mark the flush as On 2014/12/04 19:21:34, DaleCurtis wrote: > Does the bug this resolved no longer happen? This code is not needed with ProcessPendingSamples posting the FlushInternal task. The bug with hangouts does not happen anymore. I verified that https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.cc (right): https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.cc:1441: DCHECK(state_ == new_state); On 2014/12/04 19:21:34, DaleCurtis wrote: > DCHECK_EQ() Done. https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/dxva_video_decode_accelerator.h (right): https://codereview.chromium.org/765533005/diff/160001/content/common/gpu/medi... content/common/gpu/media/dxva_video_decode_accelerator.h:246: // WeakPtrFactory for posting tasks back to |this|. On 2014/12/04 19:21:34, DaleCurtis wrote: > move the WeakPtr members to the end of the variable definitions, they should > always be destructed first except in rare cases. Done.
The CQ bit was checked by ananta@chromium.org
The CQ bit was unchecked by ananta@chromium.org
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/765533005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1dd7d968e3f77ec0c90a80c944cd1ec4da8aab11 Cr-Commit-Position: refs/heads/master@{#306929} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
