Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313603004:
MediaRecorderHandler (video part) and unittests (Closed)
|
Created:
5 years, 4 months ago by mcasas Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaRecorderHandler (video part) and unittests
MediaRecorderHandler is Blink-orchestrated class implementing
MediaRecorder API (see below). It plugs together an existing
MediaStreamVideoTrack to a new VideoTrackRecorder-WebmMuxer
pair. When MSVTrack passes frames, these get encoded, muxed,
and the result is sent to Blink.
A *note on threading*: As is customary in MediaStream* and
derived classes, all configuration happens on Main Render thread
while frame manipulation and forwarding happens on Render IO
thread [1]. Moreover, all objects can be, and often are, destroyed in
asynchronous and unexpected ways from Blink. This forces
ref-counting for VideoTrackRecorder::VpxEncoder. This is the
also the reason behind the change in WebmMuxer to 2-threaded.
See DD @ https://goo.gl/vSjzC5 (*) for the plan.
(*) Used to be https://goo.gl/kreaQj
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/media_stream_video_track.cc&sq=package:chromium&type=cs&l=155&rcl=1440530828
BUG=262211
Committed: https://crrev.com/1567842287b4097350d33c14e5f795559fa008b5
Cr-Commit-Position: refs/heads/master@{#348037}
Patch Set 1 : Rebase, gyp and MediaRecorderHandlerTest #
Total comments: 27
Patch Set 2 : miu@s comments -- except thread in which ~VpxEncoder() executes. Rebase. Using blink::WebMediaRecor… #
Total comments: 16
Patch Set 3 : second round of miu@s comments #
Total comments: 4
Patch Set 4 : miu@s comments and avoiding compilation in Android platform #
Total comments: 5
Patch Set 5 : LOG() msg correction #Patch Set 6 : Moved DCHECK_EQ() #
Messages
Total messages: 36 (19 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL Please read the notes on the CL description on threading to understand why I changed (again) VideoTrackRecorder :)
Patchset #1 (id:100001) has been deleted
Sorry for the long delay. On the whole, the MediaRecorderHandler looks good. However, I don't see the need for any of the changes to video_track_recorder.* and webm_muxer.* FWICT, they should work well with MediaRecorderHandler without changes. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:64: DCHECK(!video_tracks.isEmpty()); It's possible to record MediaStreams that only have audio tracks, right? If so, this DCHECK is invalid. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:70: &media::WebmMuxer::OnEncodedVideo, base::Unretained(webm_muxer_.get())); So, if there are multiple video tracks, it looks like the video frames from different tracks will get jumbled together into a single video track in the WebmMuxer. So, does this mean you need to resurrect the "AddTrack()" method in WebmMuxer to support multiple video tracks? https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.h (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.h:33: virtual void writeData(const char* data, size_t length, bool lastInslice) = 0; Should the name of the 3rd arg be lastInSlice (capital S) here and everywhere else in the .h and .cc files? https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.h:72: bool recording_; You don't need this boolean. You could replace it with a private helper method: bool is_recording() const { return !!webm_muxer_; } https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:39: CHECK_EQ(ret, VPX_CODEC_OK) << "Failed to destroy codec"; nit: No need for the logging string. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:133: DCHECK(main_render_thread_checker_.CalledOnValidThread()); Because this is now ref-counted, there is no guarantee the dtor will be called on the main render thread. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:269: encoder_.reset(new vpx_codec_ctx_t); Why did you change this to a custom scoped ptr? It doesn't seem to affect anything except to allocate the encoder as a separate heap object. https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.cc:31: // No need to segment_.Finalize() since is not Seekable(), i.e. a live stream. What if this class is used to write a complete WebM file? (I have been using it for generating some sample videos. BTW--I have bug fixes and will send you a CR soon.) https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.h:73: base::ThreadChecker io_thread_checker_; Not sure you need the name change here, since OnEncodedVideo() can be called by any one thread. BTW--I'm not sure any of the changes in this header file are needed. You could also just clarify in the class comments that OnEncodedVideo() must be called by the same thread.
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #3 (id:220001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
miu@ PTAL Please read my comments and question on threading. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:64: DCHECK(!video_tracks.isEmpty()); On 2015/09/02 21:28:12, miu wrote: > It's possible to record MediaStreams that only have audio tracks, right? If so, > this DCHECK is invalid. Done. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:70: &media::WebmMuxer::OnEncodedVideo, base::Unretained(webm_muxer_.get())); On 2015/09/02 21:28:12, miu wrote: > So, if there are multiple video tracks, it looks like the video frames from > different tracks will get jumbled together into a single video track in the > WebmMuxer. So, does this mean you need to resurrect the "AddTrack()" method in > WebmMuxer to support multiple video tracks? I can envision the overwhelming majority of users recording a single Video track. So for the time being I believe supporting one such is not too limiting. I guess in that case is best to leave things as they are, and added a DCHECK+TODO. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.h (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.h:33: virtual void writeData(const char* data, size_t length, bool lastInslice) = 0; On 2015/09/02 21:28:13, miu wrote: > Should the name of the 3rd arg be lastInSlice (capital S) here and everywhere > else in the .h and .cc files? Done. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.h:72: bool recording_; On 2015/09/02 21:28:13, miu wrote: > You don't need this boolean. You could replace it with a private helper method: > > bool is_recording() const { return !!webm_muxer_; } Still needs to be |recording_| == false between pause() and resume(), and |webm_muxer_| exists in that period of time. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:39: CHECK_EQ(ret, VPX_CODEC_OK) << "Failed to destroy codec"; On 2015/09/02 21:28:13, miu wrote: > nit: No need for the logging string. Done. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:133: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/09/02 21:28:13, miu wrote: > Because this is now ref-counted, there is no guarantee the dtor will be called > on the main render thread. You're right. I've checked other MediaStreamVideoSinks and they don't seem to address this issue in a clean way. MediaStreamVideoRendererSink avoids it by using BindToCurrentLoop() with its OnVideoFrame(), then it goes on to using a WeakPtrFactory. VideoTrackToPepperAdapter::PpFrameReceiver also uses a BindToCurrentLoop() and a WeakPtrFactory. I could reuse that pattern here and make sure VpxEncoder is created and lives in Main Render Thread, and that would simplify WebmMuxer, which could stay single-threaded, but that would not work for the back-and-forth hop to the encoding thread, unless I also use BindToCurrentLoop. WDYT? https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:269: encoder_.reset(new vpx_codec_ctx_t); On 2015/09/02 21:28:13, miu wrote: > Why did you change this to a custom scoped ptr? It doesn't seem to affect > anything except to allocate the encoder as a separate heap object. Is due to the possible case of ctor() + dtor() without ConfigureVp8Encoding() (i.e. create-destroy with no frames received. Using this specific scoped_ptr<> does clean up automagically, which means less troubles down the road. Note that there is a unit test for this, InitializeStartStop. https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.cc:31: // No need to segment_.Finalize() since is not Seekable(), i.e. a live stream. On 2015/09/02 21:28:13, miu wrote: > What if this class is used to write a complete WebM file? (I have been using it > for generating some sample videos. BTW--I have bug fixes and will send you a CR > soon.) The mode (kLive/kFile) does not relate to File/Streaming, but to writing headers, cues and markers for seeking. Writing a Live WebM stream to file allows for playback -- I guess seeking might prove challenging. https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.h:73: base::ThreadChecker io_thread_checker_; On 2015/09/02 21:28:13, miu wrote: > Not sure you need the name change here, since OnEncodedVideo() can be called by > any one thread. Done., but in general IO thread is where the frames come from. > > BTW--I'm not sure any of the changes in this header file are needed. You could > also just clarify in the class comments that OnEncodedVideo() must be called by > the same thread. That's true. I just like to find comments on class+threading on the class definition. Not too common, but helpful. Done.
Okay, I think we're getting close now: https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:70: &media::WebmMuxer::OnEncodedVideo, base::Unretained(webm_muxer_.get())); On 2015/09/04 02:16:30, mcasas wrote: > I can envision the overwhelming majority of users > recording a single Video track. So for the time being > I believe supporting one such is not too limiting. I > guess in that case is best to leave things as they are, > and added a DCHECK+TODO. SGTM. However, since this is an issue of incomplete implementation, it's best not to crash the process. Instead, the code should try to continue normally whenever the the "audio only" or "multiple video tracks" cases are presented to this method. Instead of the DCHECKs, you could: LOG_IF(WARNING, !audio_tracks.isEmpty()) << "Recording audio tracks is not implemented for MediaStream " << identifier; LOG_IF(WARNING, video_tracks.size() > 1) << "Recording multiple video tracks is not implemented. Only recording first video track in the MediaStream " << identifier; Also, please either convert your for-loop to an if-statement, or add a break statement at the end of the block. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.h (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.h:72: bool recording_; On 2015/09/04 02:16:30, mcasas wrote: > On 2015/09/02 21:28:13, miu wrote: > > You don't need this boolean. You could replace it with a private helper > method: > > > > bool is_recording() const { return !!webm_muxer_; } > > Still needs to be |recording_| == false between pause() and > resume(), and |webm_muxer_| exists in that period of time. Acknowledged. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:133: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/09/04 02:16:30, mcasas wrote: > On 2015/09/02 21:28:13, miu wrote: > > Because this is now ref-counted, there is no guarantee the dtor will be called > > on the main render thread. > > You're right. > > I've checked other MediaStreamVideoSinks and they don't > seem to address this issue in a clean way. > ... > > WDYT? Seem to me that you can write a dtor that is able to be run on any thread: VideoTrackRecorder::VpxEncoder::~VpxEncoder() { main_task_runner_->PostTask(FROM_HERE, base::Bind(&VpxEncoder::ShutdownEncoder, base::Passed(&encoding_thread_), base::Passed(&encoder_))); } // static void VideoTrackRecorder::VpxEncoder::ShutdownEncoder( scoped_ptr<base::Thread> encoding_thread, ScopedVpxCodecCtxPtr encoder) { encoding_thread->Stop(); // Both |encoding_thread| and |encoder| will be destroyed at end-of-scope. } ...and now I will stop harping on you for writing the custom scoped_ptr<> for the VP8 encoder context, since it is actually needed now. ;-) https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:269: encoder_.reset(new vpx_codec_ctx_t); On 2015/09/04 02:16:30, mcasas wrote: > On 2015/09/02 21:28:13, miu wrote: > > Why did you change this to a custom scoped ptr? It doesn't seem to affect > > anything except to allocate the encoder as a separate heap object. > > Is due to the possible case of ctor() + dtor() without > ConfigureVp8Encoding() (i.e. create-destroy with no > frames received. Using this specific scoped_ptr<> > does clean up automagically, which means less > troubles down the road. Note that there is a unit > test for this, InitializeStartStop. But you have IsInitialized() to tell you whether you need to call vpx_codec_destroy(). In any case, if you follow my advice for the dtor, this is now a non-issue. https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.cc:31: // No need to segment_.Finalize() since is not Seekable(), i.e. a live stream. On 2015/09/04 02:16:30, mcasas wrote: > On 2015/09/02 21:28:13, miu wrote: > > What if this class is used to write a complete WebM file? (I have been using > it > > for generating some sample videos. BTW--I have bug fixes and will send you a > CR > > soon.) > > The mode (kLive/kFile) does not relate to File/Streaming, but > to writing headers, cues and markers for seeking. Writing a Live > WebM stream to file allows for playback -- I guess seeking > might prove challenging. Would it break anything to include the call to Finalize()? If not, please include it. I'm using it. ;-) https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:31: NOTIMPLEMENTED(); Shouldn't this return true iff mimeType == "video/vp8" ? https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:40: // TODO(mcasas): check canSupportMimeType(mimeType) is true. Ditto here (return false iff mimeType != "video/vp8")? https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:66: // TODO(mcasas): Add audio_tracks and update this DCHECK() correspondingly. crbug link? https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:69: // TOOD(mcasas): The muxer API supports only one video track. Extend it to crbug link here too https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:44: typedef scoped_ptr<vpx_codec_ctx_t, VpxCodecDeleter> ScopedVpxCodec; naming nit: ScopedVpxCodecCtxPtr https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:62: // This class needs to be ref-counted to avoid destruction (of |encoder_| in This is highly misleading because it isn't the reason ref-counting is needed. Please change it to something like: "This class must be ref-counted because MediaStreamVideoSink will hold a reference to it (via the callback given to AddToVideoTrack()), and it's unknown when exactly it will release that reference." https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:105: // Thread for encoding. Active as long as VpxEncoder exists. All variables Comment needs adjustment: "Active for the lifetime of VTR::VpxEncoder." https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:110: // |encoder_| is a special scoped pointer to guarantee proper destruction. Please add: Should only be accessed on encoding_thread_.
miu@ PTAL. I like the new VpxEncoder destructor. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:70: &media::WebmMuxer::OnEncodedVideo, base::Unretained(webm_muxer_.get())); On 2015/09/04 21:48:52, miu wrote: > On 2015/09/04 02:16:30, mcasas wrote: > > I can envision the overwhelming majority of users > > recording a single Video track. So for the time being > > I believe supporting one such is not too limiting. I > > guess in that case is best to leave things as they are, > > and added a DCHECK+TODO. > > SGTM. > > However, since this is an issue of incomplete implementation, it's best not to > crash the process. Instead, the code should try to continue normally whenever > the the "audio only" or "multiple video tracks" cases are presented to this > method. Instead of the DCHECKs, you could: > > LOG_IF(WARNING, !audio_tracks.isEmpty()) << "Recording audio tracks is not > implemented for MediaStream " << identifier; > > LOG_IF(WARNING, video_tracks.size() > 1) << "Recording multiple video tracks > is not implemented. Only recording first video track in the MediaStream " << > identifier; > > Also, please either convert your for-loop to an if-statement, or add a break > statement at the end of the block. Done. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:133: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/09/04 21:48:52, miu wrote: > On 2015/09/04 02:16:30, mcasas wrote: > > On 2015/09/02 21:28:13, miu wrote: > > > Because this is now ref-counted, there is no guarantee the dtor will be > called > > > on the main render thread. > > > > You're right. > > > > I've checked other MediaStreamVideoSinks and they don't > > seem to address this issue in a clean way. > > ... > > > > WDYT? > > Seem to me that you can write a dtor that is able to be run on any thread: > > VideoTrackRecorder::VpxEncoder::~VpxEncoder() { > main_task_runner_->PostTask(FROM_HERE, > base::Bind(&VpxEncoder::ShutdownEncoder, > base::Passed(&encoding_thread_), > base::Passed(&encoder_))); > } > > // static > void VideoTrackRecorder::VpxEncoder::ShutdownEncoder( > scoped_ptr<base::Thread> encoding_thread, > ScopedVpxCodecCtxPtr encoder) { > encoding_thread->Stop(); > // Both |encoding_thread| and |encoder| will be destroyed at end-of-scope. > } > > ...and now I will stop harping on you for writing the custom scoped_ptr<> for > the VP8 encoder context, since it is actually needed now. ;-) Done. https://codereview.chromium.org/1313603004/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:269: encoder_.reset(new vpx_codec_ctx_t); On 2015/09/04 21:48:52, miu wrote: > On 2015/09/04 02:16:30, mcasas wrote: > > On 2015/09/02 21:28:13, miu wrote: > > > Why did you change this to a custom scoped ptr? It doesn't seem to affect > > > anything except to allocate the encoder as a separate heap object. > > > > Is due to the possible case of ctor() + dtor() without > > ConfigureVp8Encoding() (i.e. create-destroy with no > > frames received. Using this specific scoped_ptr<> > > does clean up automagically, which means less > > troubles down the road. Note that there is a unit > > test for this, InitializeStartStop. > > But you have IsInitialized() to tell you whether you need to call > vpx_codec_destroy(). In any case, if you follow my advice for the dtor, this is > now a non-issue. Acknowledged. https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/120001/media/capture/webm_mux... media/capture/webm_muxer.cc:31: // No need to segment_.Finalize() since is not Seekable(), i.e. a live stream. On 2015/09/04 21:48:52, miu wrote: > On 2015/09/04 02:16:30, mcasas wrote: > > On 2015/09/02 21:28:13, miu wrote: > > > What if this class is used to write a complete WebM file? (I have been > using > > it > > > for generating some sample videos. BTW--I have bug fixes and will send you > a > > CR > > > soon.) > > > > The mode (kLive/kFile) does not relate to File/Streaming, but > > to writing headers, cues and markers for seeking. Writing a Live > > WebM stream to file allows for playback -- I guess seeking > > might prove challenging. > > Would it break anything to include the call to Finalize()? If not, please > include it. I'm using it. ;-) Done. Left the comment though. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:31: NOTIMPLEMENTED(); On 2015/09/04 21:48:52, miu wrote: > Shouldn't this return true iff mimeType == "video/vp8" ? Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:40: // TODO(mcasas): check canSupportMimeType(mimeType) is true. On 2015/09/04 21:48:52, miu wrote: > Ditto here (return false iff mimeType != "video/vp8")? Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:66: // TODO(mcasas): Add audio_tracks and update this DCHECK() correspondingly. On 2015/09/04 21:48:52, miu wrote: > crbug link? Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:69: // TOOD(mcasas): The muxer API supports only one video track. Extend it to On 2015/09/04 21:48:52, miu wrote: > crbug link here too Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:44: typedef scoped_ptr<vpx_codec_ctx_t, VpxCodecDeleter> ScopedVpxCodec; On 2015/09/04 21:48:52, miu wrote: > naming nit: ScopedVpxCodecCtxPtr Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:62: // This class needs to be ref-counted to avoid destruction (of |encoder_| in On 2015/09/04 21:48:52, miu wrote: > This is highly misleading because it isn't the reason ref-counting is needed. > > Please change it to something like: "This class must be ref-counted because > MediaStreamVideoSink will hold a reference to it (via the callback given to > AddToVideoTrack()), and it's unknown when exactly it will release that > reference." Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:105: // Thread for encoding. Active as long as VpxEncoder exists. All variables On 2015/09/04 21:48:52, miu wrote: > Comment needs adjustment: "Active for the lifetime of VTR::VpxEncoder." Done. https://codereview.chromium.org/1313603004/diff/240001/content/renderer/media... content/renderer/media/video_track_recorder.cc:110: // |encoder_| is a special scoped pointer to guarantee proper destruction. On 2015/09/04 21:48:52, miu wrote: > Please add: Should only be accessed on encoding_thread_. Done. (Note the comment in l.105-106)
lgtm % one logic flaw to fix: https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:71: // TODO(mcasas): Add audio_tracks and update this DCHECK() correspondingly, Comment refers to a DCHECK that is now gone. https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:79: return false; You should only return false if video_tracks.IsEmpty(). Otherwise, you *do* want to continue and record the first of multiple video tracks.
Patchset #4 (id:280001) has been deleted
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@ Owners RS for media/capture or PTAL :) https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:71: // TODO(mcasas): Add audio_tracks and update this DCHECK() correspondingly, On 2015/09/08 20:24:40, miu wrote: > Comment refers to a DCHECK that is now gone. Done. https://codereview.chromium.org/1313603004/diff/260001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:79: return false; On 2015/09/08 20:24:40, miu wrote: > You should only return false if video_tracks.IsEmpty(). Otherwise, you *do* > want to continue and record the first of multiple video tracks. Done.
https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... media/capture/webm_muxer.cc:65: DCHECK segment isn't already initialized? Or is repeat initialization okay?
lgtm % something I missed: https://codereview.chromium.org/1313603004/diff/300001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/300001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:74: LOG(WARNING) << "Recording audio tracks is not implemented"; s/Recording audio/Recording no video/
https://codereview.chromium.org/1313603004/diff/300001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1313603004/diff/300001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:74: LOG(WARNING) << "Recording audio tracks is not implemented"; On 2015/09/09 00:07:10, miu wrote: > s/Recording audio/Recording no video/ Done. https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... media/capture/webm_muxer.cc:65: On 2015/09/09 00:04:26, DaleCurtis wrote: > DCHECK segment isn't already initialized? Or is repeat initialization okay? Hmm mkvmuxer::Segment doesn't offer any method to check if it has been initialized or not. From the implementation (that I shouldn't be peeking on) it seems fine. In any case the marker on WebmMuxer side that AddVideoTrack() has been initialized is in l.74, |track_index_| must be zero in the non-initialized state.
lgtm % moving the dcheck https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1313603004/diff/300001/media/capture/webm_mux... media/capture/webm_muxer.cc:65: On 2015/09/09 00:22:15, mcasas wrote: > On 2015/09/09 00:04:26, DaleCurtis wrote: > > DCHECK segment isn't already initialized? Or is repeat initialization okay? > > Hmm mkvmuxer::Segment doesn't offer any method to > check if it has been initialized or not. From the implementation > (that I shouldn't be peeking on) it seems fine. In any case > the marker on WebmMuxer side that AddVideoTrack() > has been initialized is in l.74, |track_index_| must be zero > in the non-initialized state. Seems you should DCHECK that at the top of this function then.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1313603004/#ps340001 (title: "Moved DCHECK_EQ()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313603004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313603004/340001
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_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313603004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313603004/340001
Message was sent while issue was closed.
Committed patchset #6 (id:340001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1567842287b4097350d33c14e5f795559fa008b5 Cr-Commit-Position: refs/heads/master@{#348037}
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1567842287b4097350d33c14e5f795559fa008b5 Cr-Commit-Position: refs/heads/master@{#348037} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
