Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000003002:
Initialize based on frame sizes in VideoTrackRecorder (Closed)
|
Created:
4 years, 7 months ago by emircan Modified:
4 years, 5 months ago Reviewers:
mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mrgpu2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize based on frame sizes in VideoTrackRecorder
This CL modifies the current behavior of VideoTrackRecorder, such that
instead of initializing the underlying |encoder_| in ctor, we wait till the
first frame arrives. Based on the frame size, we choose the appropriate
HW/SW encoder.
BUG=608385
TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html
Committed: https://crrev.com/0924bc046223e0c9eac949c35c69844c4bfd7513
Cr-Commit-Position: refs/heads/master@{#404966}
Patch Set 1 : Rebase. #
Total comments: 8
Patch Set 2 : #Patch Set 3 : Retake. #
Total comments: 9
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 1
Patch Set 6 : Retake using lock. #Patch Set 7 : Retake using bool. #
Total comments: 2
Patch Set 8 : #Patch Set 9 : Drop first frames. #
Total comments: 9
Patch Set 10 : #
Total comments: 2
Messages
Total messages: 39 (11 generated)
Description was changed from ========== Check first frame. BUG= ========== to ========== Initialize based on frame sizes in VideoTrackRecorder This CL modifies the current behavior of VideoTrackRecorder, such that instead of initializing the underlying |encoder_| in ctor, we wait till the first frame arrives. Based on the frame size, we choose the appropriate HW/SW encoder. BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org
PTAL.
Patchset #1 (id:1) has been deleted
PING. I rebased to point at ToT.
Some comments, potentially good news. https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:948: base::Bind(&VideoTrackRecorder::StartFrameEncode, base::Unretained(this)), Why not use weak_ptr_factory_.GetWeakPtr() here? https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:974: weak_ptr_factory_.GetWeakPtr(), frame, capture_timestamp)); Actually, it might be easier than all this thread dancing. |encoder_| lives on |origin_task_runner_| (that we know to be IO thread). So what about a) add a comment that |encoder_| member lives on IO |origin_task_runner_| b) not do this PostTask c) Simplify InitializeEncoder (rename appropriately?) d) in ~VideoTrackRecorder(), use sth like DeleteSoon(encoder_.release()) e) and update Pause() and Resume() to trivial PostTasks ? f) Update comments in Encoder about its threading model (now it'll be fully living on the construction thread). Essentially, moving the whole |encoder_| management and interaction into the IO thread. Then, probably, you can simplify |status_| into a boolean |is_initialized_| or, even better, remove it in favour of a method boolean IsIinitialized() const { return !!encoder_; } https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.h:13: #include "base/single_thread_task_runner.h" Forward declare SingleThreadTaskRunner https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.h:76: scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_; const?
https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:948: base::Bind(&VideoTrackRecorder::StartFrameEncode, base::Unretained(this)), On 2016/05/25 16:42:47, mcasas wrote: > Why not use weak_ptr_factory_.GetWeakPtr() here? Done. https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:974: weak_ptr_factory_.GetWeakPtr(), frame, capture_timestamp)); On 2016/05/25 16:42:47, mcasas wrote: > Actually, it might be easier than all this thread > dancing. > > |encoder_| lives on |origin_task_runner_| > (that we know to be IO thread). So what about > a) add a comment that |encoder_| member lives > on IO |origin_task_runner_| > b) not do this PostTask > c) Simplify InitializeEncoder (rename appropriately?) > d) in ~VideoTrackRecorder(), use sth like > DeleteSoon(encoder_.release()) > e) and update Pause() and Resume() to trivial > PostTasks ? > f) Update comments in Encoder about its > threading model (now it'll be fully living on the > construction thread). > > Essentially, moving the whole |encoder_| management > and interaction into the IO thread. > > Then, probably, you can simplify |status_| into a > boolean |is_initialized_| or, even better, remove it > in favour of a method > boolean IsIinitialized() const { return !!encoder_; } VEAEncoder needs to access RenderThreadImpl methods and those are only accessible via Main Render thread. See https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r.... I understand that these thread hops look dangerous, but we need Main thread for creation of objects and IO thread for interaction. WDYT? https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.h:13: #include "base/single_thread_task_runner.h" On 2016/05/25 16:42:47, mcasas wrote: > Forward declare SingleThreadTaskRunner Done. https://codereview.chromium.org/2000003002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.h:76: scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_; On 2016/05/25 16:42:47, mcasas wrote: > const? Done.
Patchset #3 (id:60001) has been deleted
PTAL. I rewrote this CL so that: - There is no thread jump or call forwarding for each frame. We only do it in the initialization, and reconnect to the track via the correct encoder. - Added unit tests to account for size.
Looking good, just a few comments to get you going. https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:954: base::Unretained(this)), weak_ptr_factory_.GetWeakPtr() https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:1004: weak_ptr_factory_.GetWeakPtr(), frame, capture_time)); I like the new model of PostTask() here, then DisconnectFromTrack() - reconnect to VTR::Encoder::StartFrameEncode at the end of the other method. What if we receive >1 frame here before InitializeEncodeTask() is finished? I think we need a one-shot Callback with InitializeEncoderTask inside. https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.h:64: base::TimeTicks capture_time); Shift four spaces right. s/InitializeEncoderTask/InitializeEncoderOnMainThread/ ?
https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:954: base::Unretained(this)), On 2016/06/08 18:51:24, mcasas wrote: > weak_ptr_factory_.GetWeakPtr() Weak_ptr would be attached to the IO thread then? https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:1004: weak_ptr_factory_.GetWeakPtr(), frame, capture_time)); On 2016/06/08 18:51:24, mcasas wrote: > I like the new model of PostTask() here, > then DisconnectFromTrack() - reconnect to > VTR::Encoder::StartFrameEncode at the end > of the other method. > > What if we receive >1 frame here before > InitializeEncodeTask() is finished? I think we need > a one-shot Callback with InitializeEncoderTask > inside. I initially had a one-shot callback where I add media::BindToCurrentLoop in l.953. However, I still need to set |origin_task_runner_| so I can send the first frame for encode on the right thread, see l.1044. You are right about this being called >1 times. I will add a async waiter so that if it is called for the second time, i can directly send it to |encoder_|. https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.h:64: base::TimeTicks capture_time); On 2016/06/08 18:51:24, mcasas wrote: > Shift four spaces right. > > s/InitializeEncoderTask/InitializeEncoderOnMainThread/ ? Done.
https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:954: base::Unretained(this)), On 2016/06/09 03:03:43, emircan wrote: > On 2016/06/08 18:51:24, mcasas wrote: > > weak_ptr_factory_.GetWeakPtr() > > Weak_ptr would be attached to the IO thread then? Yes, the WeakPtrFactory (and associated pointers) get attached to the appropriate thread on first use [1], however in this case that'd pose a problem for destruction. However, using a base::Unretained makes me feel uneasy. What about: MediaStreamVideoSink::ConnectToTrack( track_, media::BindToCurrentLoop( base::Bind(&VideoTrackRecorder::InitializeEncoder, weak_ptr_factory_.GetWeakPtr()))); Which will bounce us back to main thread, and allow for using |weak_ptr_factory_| ? [1] https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?sq=package:chromi... https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.h:64: base::TimeTicks capture_time); On 2016/06/09 03:03:43, emircan wrote: > On 2016/06/08 18:51:24, mcasas wrote: > > Shift four spaces right. > > > > s/InitializeEncoderTask/InitializeEncoderOnMainThread/ ? And this? However: irrelevant, after my other series of comments. > > Done. https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1013: } Assuming you take in my suggestion before using media::BindToCurrentLoop (which I strongly encourage you to), we wouldn't need the PostTask in l.1006-1009, but then we'd need a PostTask here instead. However, IIUC, by the time we're done creating |encoder_| via InitializeEncoderTask() l.1060, the Track will already be delivering to VideoTrackEncoder::Encoder::StartFrameEncode(), correct? Moreover, we should not use |encoder_| from two threads without a lock either. So, what I propose is the following pseudo code, based on BindToCurrentLoop(): - when the first frame arrives (on main thread), then simply do the InitializeEncoderTask() steps. - at the end of those steps, we are reconnecting the Track to deliver to VTR::Encoder, which is fine. - since MessageLoops are not reentrant, if there were still frames in flight they would be queued up in the BindToCurrentLoop posting of tasks, and they'd come up _after_ these initialization steps. In that case, just PostTask those to |encoder_| StartFrameEncode(). SG?
https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/80001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:954: base::Unretained(this)), On 2016/06/12 08:56:34, mcasas wrote: > On 2016/06/09 03:03:43, emircan wrote: > > On 2016/06/08 18:51:24, mcasas wrote: > > > weak_ptr_factory_.GetWeakPtr() > > > > Weak_ptr would be attached to the IO thread then? > > Yes, the WeakPtrFactory (and associated pointers) > get attached to the appropriate thread on first use [1], > however in this case that'd pose a problem for > destruction. However, using a base::Unretained > makes me feel uneasy. What about: > > MediaStreamVideoSink::ConnectToTrack( > track_, media::BindToCurrentLoop( > base::Bind(&VideoTrackRecorder::InitializeEncoder, > weak_ptr_factory_.GetWeakPtr()))); > > Which will bounce us back to main thread, > and allow for using |weak_ptr_factory_| ? > > > [1] > https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?sq=package:chromi... As I said in my previous comment, "I initially had a one-shot callback where I add media::BindToCurrentLoop in l.953. However, I still need to set |origin_task_runner_| so I can send the first frame for encode on the right thread, see l.1044." https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1013: } On 2016/06/12 08:56:34, mcasas wrote: > Assuming you take in my suggestion before > using media::BindToCurrentLoop (which I strongly > encourage you to), we wouldn't need the PostTask > in l.1006-1009, but then we'd need a PostTask here > instead. However, IIUC, by the time we're done > creating |encoder_| via InitializeEncoderTask() l.1060, > the Track will already be delivering to > VideoTrackEncoder::Encoder::StartFrameEncode(), > correct? > > Moreover, we should not use |encoder_| from > two threads without a lock either. > > So, what I propose is the following pseudo code, > based on BindToCurrentLoop(): > - when the first frame arrives (on main thread), > then simply do the InitializeEncoderTask() steps. > - at the end of those steps, we are reconnecting > the Track to deliver to VTR::Encoder, which is fine. > - since MessageLoops are not reentrant, if there > were still frames in flight they would be queued up > in the BindToCurrentLoop posting of tasks, and > they'd come up _after_ these initialization steps. > In that case, just PostTask those to |encoder_| > StartFrameEncode(). > > SG? I agree that it is the right model if we are using BindToCurrentLoop. But still my only concern is losing that precious first frame for encode. We need to hold onto |origin_task_runner| so that we can send it for encode on that thread. Therefore, we still need to post tasks in between. WDYT?
LGTM % comments below. https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1013: } On 2016/06/13 19:55:40, emircan wrote: > On 2016/06/12 08:56:34, mcasas wrote: > > Assuming you take in my suggestion before > > using media::BindToCurrentLoop (which I strongly > > encourage you to), we wouldn't need the PostTask > > in l.1006-1009, but then we'd need a PostTask here > > instead. However, IIUC, by the time we're done > > creating |encoder_| via InitializeEncoderTask() l.1060, > > the Track will already be delivering to > > VideoTrackEncoder::Encoder::StartFrameEncode(), > > correct? > > > > Moreover, we should not use |encoder_| from > > two threads without a lock either. > > > > So, what I propose is the following pseudo code, > > based on BindToCurrentLoop(): > > - when the first frame arrives (on main thread), > > then simply do the InitializeEncoderTask() steps. > > - at the end of those steps, we are reconnecting > > the Track to deliver to VTR::Encoder, which is fine. > > - since MessageLoops are not reentrant, if there > > were still frames in flight they would be queued up > > in the BindToCurrentLoop posting of tasks, and > > they'd come up _after_ these initialization steps. > > In that case, just PostTask those to |encoder_| > > StartFrameEncode(). > > > > SG? > > I agree that it is the right model if we are using BindToCurrentLoop. But still > my only concern is losing that precious first frame for encode. We need to hold > onto |origin_task_runner| so that we can send it for encode on that thread. > Therefore, we still need to post tasks in between. WDYT? Reluctantly, because is ugly, I admit that this thread-hopper-monster is the only solution. However, I think we can remove the else{ ...} since, efffectively, l.1010 is going to lock the IO thread frame delivery, so as long as there is no Signal on the WaitableEvent, PC would not move from l.1010, and when it does, we would have executed already l.1060, so frames would be going to |encoder_|. We can turn instead this if() else into a DCHECK(!encoder); // WaitableEvent, PostTask etc DCHECK(encoder); https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1059: // StartFrameEncode() will be called on Render IO thread. nit: s/on Render IO/on |origin_task_runner_|/ https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.h:65: void InitializeEncoder(const scoped_refptr<media::VideoFrame>& frame, Can we call this method ReceiveFirstFrameOnIOThread and the next one InitializeEncoderOnMainThread? (note that I said this latter on before).
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1013: } On 2016/06/23 00:27:36, mcasas wrote: > On 2016/06/13 19:55:40, emircan wrote: > > On 2016/06/12 08:56:34, mcasas wrote: > > > Assuming you take in my suggestion before > > > using media::BindToCurrentLoop (which I strongly > > > encourage you to), we wouldn't need the PostTask > > > in l.1006-1009, but then we'd need a PostTask here > > > instead. However, IIUC, by the time we're done > > > creating |encoder_| via InitializeEncoderTask() l.1060, > > > the Track will already be delivering to > > > VideoTrackEncoder::Encoder::StartFrameEncode(), > > > correct? > > > > > > Moreover, we should not use |encoder_| from > > > two threads without a lock either. > > > > > > So, what I propose is the following pseudo code, > > > based on BindToCurrentLoop(): > > > - when the first frame arrives (on main thread), > > > then simply do the InitializeEncoderTask() steps. > > > - at the end of those steps, we are reconnecting > > > the Track to deliver to VTR::Encoder, which is fine. > > > - since MessageLoops are not reentrant, if there > > > were still frames in flight they would be queued up > > > in the BindToCurrentLoop posting of tasks, and > > > they'd come up _after_ these initialization steps. > > > In that case, just PostTask those to |encoder_| > > > StartFrameEncode(). > > > > > > SG? > > > > I agree that it is the right model if we are using BindToCurrentLoop. But > still > > my only concern is losing that precious first frame for encode. We need to > hold > > onto |origin_task_runner| so that we can send it for encode on that thread. > > Therefore, we still need to post tasks in between. WDYT? > > Reluctantly, because is ugly, I admit that this > thread-hopper-monster is the only solution. > However, I think we can remove the else{ ...} > since, efffectively, l.1010 is going to lock the > IO thread frame delivery, so as long as there is > no Signal on the WaitableEvent, PC would not > move from l.1010, and when it does, we would > have executed already l.1060, so frames would > be going to |encoder_|. We can turn instead this > if() else into a > DCHECK(!encoder); > // WaitableEvent, PostTask etc > DCHECK(encoder); Done. https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.h:65: void InitializeEncoder(const scoped_refptr<media::VideoFrame>& frame, On 2016/06/23 00:27:36, mcasas wrote: > Can we call this method > ReceiveFirstFrameOnIOThread > > and the next one > InitializeEncoderOnMainThread? > (note that I said this latter on before). Done.
PTAL again. WaitableEvent solution that I had on PS#5 had flakiness issues on some bots. Digging deeper I found that RenderThreadImpl::GetGpuFactories() might lead to using IO thread. Therefore, waiting resulted in a deadlock in some cases. I moved onto a Lock based solution instead to serialize ReceiveFirstFrameOnIOThread and InitializeEncoderOnMainThread calls, which turned bots green.
On 2016/06/29 02:15:57, emircan wrote: > PTAL again. > > WaitableEvent solution that I had on PS#5 had flakiness issues on some bots. > Digging deeper I found that RenderThreadImpl::GetGpuFactories() might lead to > using IO thread. Therefore, waiting resulted in a deadlock in some cases. > > I moved onto a Lock based solution instead to serialize > ReceiveFirstFrameOnIOThread and InitializeEncoderOnMainThread calls, which > turned bots green. PS6 still lgtm, as a matter of fact, more so :)
https://codereview.chromium.org/2000003002/diff/140001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/140001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1016: DCHECK(encoder_); Keep this guy, right?
After offline discussion, using a bool to mark that initialization task is posted. PTAL.
After offline discussion, using a bool to mark that initialization task is posted. PTAL.
https://codereview.chromium.org/2000003002/diff/180001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/180001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1005: // until the new sink is connected to th track. s/th/the/ Also you could say "...and drop eventual subsequent frames until the new sink..." https://codereview.chromium.org/2000003002/diff/180001/content/renderer/media... content/renderer/media/video_track_recorder.cc:1056: encoder_, frame, capture_time)); I'm not sure we should do this. Rationale: If this method takes a while, we'd ignore a few incoming frames in ReceiveFirstFrameOnIOThread() before |encoder_| is initialized; in that case we could end up in a scenario where the first frame is encoded (because of this line) and the next one being encoded comes way later in time --> not good for encoders since that'd kill temporal redundancy. I think we should simply drop the first frame, and let |encoder_| the first available frame after it does the MSVSink::ConnectToTrack(). And if we follow that rationale, we only need frame->visible_rect() in this method. WDYT?
PTAL. As we discussed offline, I rearranged such that frames until encoder is initialized would be all dropped.
https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.cc:407: DCHECK_GE(size.width(), kVEAEncoderMinResolutionWidth); DCHECK_GE(size.height(), kVEAEncoderMinResolutionHeight); ? https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.h:76: const int32_t bits_per_second_; Don't extend this class' inner state (i.e. # member variables): instead, bind these variables from the constructor VideoTrackRecorder() to InitializeEncoder(). https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.h:81: base::WeakPtrFactory<VideoTrackRecorder> weak_ptr_factory_; This is not used in favour of base::Unretained(this) to PostTask() to InitializeEncoder() in constructor. I'd say use it because it's more self documenting, and we won't depend on MSVSink::DisconnectFromTrack() specifics, but I'll leave it to you. https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder_unittest.cc:51: gfx::Size(640, 480)}; Link these two to |kVEAEncoderMinResolutionWidth| and |kVEAEncoderMinResolutionHeight|, e.g. // Selected resolutions that will be too small or OK for using a VEA. const gfx::Size kTrackRecorderTestSize[] = { gfx::Size(kVEAEncoderMinResolutionWidth / 2, kVEAEncoderMinResolutionHeight / 2), gfx::Size(kVEAEncoderMinResolutionWidth, kVEAEncoderMinResolutionHeight)}; (Assuming you make them accessible somehow). https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder_unittest.cc:112: base::MessageLoopForUI message_loop_; Restore const?
https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.cc:407: On 2016/07/08 22:59:27, mcasas wrote: > DCHECK_GE(size.width(), kVEAEncoderMinResolutionWidth); > DCHECK_GE(size.height(), kVEAEncoderMinResolutionHeight); > ? Done. https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.h:76: const int32_t bits_per_second_; On 2016/07/08 22:59:27, mcasas wrote: > Don't extend this class' inner state (i.e. # member > variables): instead, bind these variables from the > constructor VideoTrackRecorder() to InitializeEncoder(). Done. However, I still need to hold onto this info for OnVideoFrameForTesting calls. I will bind all these in a callback for that purpose. https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder.h:81: base::WeakPtrFactory<VideoTrackRecorder> weak_ptr_factory_; On 2016/07/08 22:59:27, mcasas wrote: > This is not used in favour of base::Unretained(this) > to PostTask() to InitializeEncoder() in constructor. > I'd say use it because it's more self documenting, > and we won't depend on MSVSink::DisconnectFromTrack() > specifics, but I'll leave it to you. Done. https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2000003002/diff/220001/content/renderer/media... content/renderer/media/video_track_recorder_unittest.cc:51: gfx::Size(640, 480)}; On 2016/07/08 22:59:27, mcasas wrote: > Link these two to |kVEAEncoderMinResolutionWidth| > and |kVEAEncoderMinResolutionHeight|, e.g. > > // Selected resolutions that will be too small or OK for using a VEA. > const gfx::Size kTrackRecorderTestSize[] = { > gfx::Size(kVEAEncoderMinResolutionWidth / 2, > kVEAEncoderMinResolutionHeight / 2), > gfx::Size(kVEAEncoderMinResolutionWidth, kVEAEncoderMinResolutionHeight)}; > > (Assuming you make them accessible somehow). I put them in .h file in video_track_recorder namespace to not spam the rest.
LTGTM https://codereview.chromium.org/2000003002/diff/240001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.h:83: initialize_encoder_callback_; Make const and put it in the initializer list instead of assigning it in l.955.
https://codereview.chromium.org/2000003002/diff/240001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/2000003002/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.h:83: initialize_encoder_callback_; On 2016/07/12 15:41:12, mcasas wrote: > Make const and put it in the initializer list > instead of assigning it in l.955. I cannot. I use weak_ptr_factory_.GetWeakPtr() and it must be the last member in the initializer list.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2000003002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initialize based on frame sizes in VideoTrackRecorder This CL modifies the current behavior of VideoTrackRecorder, such that instead of initializing the underlying |encoder_| in ctor, we wait till the first frame arrives. Based on the frame size, we choose the appropriate HW/SW encoder. BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ========== to ========== Initialize based on frame sizes in VideoTrackRecorder This CL modifies the current behavior of VideoTrackRecorder, such that instead of initializing the underlying |encoder_| in ctor, we wait till the first frame arrives. Based on the frame size, we choose the appropriate HW/SW encoder. BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Initialize based on frame sizes in VideoTrackRecorder This CL modifies the current behavior of VideoTrackRecorder, such that instead of initializing the underlying |encoder_| in ctor, we wait till the first frame arrives. Based on the frame size, we choose the appropriate HW/SW encoder. BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ========== to ========== Initialize based on frame sizes in VideoTrackRecorder This CL modifies the current behavior of VideoTrackRecorder, such that instead of initializing the underlying |encoder_| in ctor, we wait till the first frame arrives. Based on the frame size, we choose the appropriate HW/SW encoder. BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html Committed: https://crrev.com/0924bc046223e0c9eac949c35c69844c4bfd7513 Cr-Commit-Position: refs/heads/master@{#404966} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0924bc046223e0c9eac949c35c69844c4bfd7513 Cr-Commit-Position: refs/heads/master@{#404966} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
