|
|
Created:
5 years, 5 months ago by mcasas Modified:
5 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaStream: Adding VideoTrackRecorder class and unittests
VTR is in charge of grabbing VideoFrames, encoding them on
IO thread and sending them forward to the registered client
(which will be in time a WebmMuxer crrev.com/1225123006).
See DD @ https://goo.gl/vSjzC5 (*) for the plan and
https://codereview.chromium.org/1211973012/ for a
hack of all Chrome parts.
(*) Used to be https://goo.gl/kreaQj
BUG=262211
Committed: https://crrev.com/a466bd2cc32a9d5bcba758473ebd993a296f7d6d
Cr-Commit-Position: refs/heads/master@{#345437}
Patch Set 1 #
Total comments: 13
Patch Set 2 : tomfinegan@ comments, added a capture thread #
Total comments: 23
Patch Set 3 : miu@s comments (except param piggybacking and threading stuff) #
Total comments: 2
Patch Set 4 : Thread cleanup and niklase@ comment #
Total comments: 2
Patch Set 5 : miu@s comment on OnFirstFrameCB and OnEncodedVideoCB #
Total comments: 1
Patch Set 6 : Moved |track_index| and timestamp mgmt from VideoTrackRecorder into WebmMuxer #
Total comments: 33
Patch Set 7 : miu@s comments; rebase; reverted unrelated code cleanups #
Total comments: 6
Patch Set 8 : miu@s comments #
Total comments: 4
Patch Set 9 : miu@s nits #
Total comments: 6
Patch Set 10 : dalecurtis@ comments #
Dependent Patchsets: Messages
Total messages: 69 (34 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:10005) has been deleted
Patchset #1 (id:90001) has been deleted
Patchset #1 (id:110001) has been deleted
Patchset #1 (id:130001) has been deleted
Patchset #2 (id:170001) has been deleted
Patchset #1 (id:150001) has been deleted
Patchset #1 (id:190001) has been deleted
Patchset #1 (id:210001) has been deleted
mcasas@chromium.org changed reviewers: + miu@chromium.org, tomfinegan@chromium.org
tomfinegan@ / miu@ PTAL at: video_track_recorder.cc
Patchset #1 (id:230001) has been deleted
https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:33: CHECK_EQ(VPX_CODEC_OK, ret) << "Failed to destroy codec"; Won't this kill a tab? Killing a tab for a weird vpx failure seems like overkill. https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* codec_context) { A static method in an anon namespace seems unnecessary. One or the other should suffice, no? https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:59: codec_config->g_timebase.den = base::Time::kMillisecondsPerSecond * 90; 90000? RTP? https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:66: codec_config->kf_max_dist = 10000; note that you can end up with more keyframes than just the one (I assume you know this; just being pedantic) https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:67: One more config flag that should always be set by real time encoders: g_lag_in_frames. Set it to 0.
Patchset #1 (id:250001) has been deleted
guys PTAL. https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:33: CHECK_EQ(VPX_CODEC_OK, ret) << "Failed to destroy codec"; On 2015/07/23 22:18:39, Tom Finegan wrote: > Won't this kill a tab? Killing a tab for a weird vpx failure seems like > overkill. Yes, changed. Btw this comes from [1], eternal glory to the one that fills in a bug about it :) [1] https://code.google.com/p/chromium/codesearch#chromium/src/remoting/codec/sco... https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* codec_context) { On 2015/07/23 22:18:39, Tom Finegan wrote: > A static method in an anon namespace seems unnecessary. One or the other should > suffice, no? It would but content rules "encourage" putting static functions and ad hoc classes in an anonymous namespace _inside_ content namespace (see for instance video_track_adapter.cc or media_stream_video_capturer_source.cc). Note that this is different from media conventions, where everything is piled up in media namespace, not in an anonymous namespace either inside or outside of the former. https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:59: codec_config->g_timebase.den = base::Time::kMillisecondsPerSecond * 90; On 2015/07/23 22:18:38, Tom Finegan wrote: > 90000? RTP? Might have taken it inadvertently, removed :) https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:66: codec_config->kf_max_dist = 10000; On 2015/07/23 22:18:39, Tom Finegan wrote: > note that you can end up with more keyframes than just the one (I assume you > know this; just being pedantic) Hmmkay, should I add a note? https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:67: On 2015/07/23 22:18:39, Tom Finegan wrote: > One more config flag that should always be set by real time encoders: > g_lag_in_frames. Set it to 0. Done.
https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* codec_context) { On 2015/07/24 15:00:10, mcasas wrote: > On 2015/07/23 22:18:39, Tom Finegan wrote: > > A static method in an anon namespace seems unnecessary. One or the other > should > > suffice, no? > > It would but content rules "encourage" putting static functions > and ad hoc classes in an anonymous namespace _inside_ > content namespace (see for instance video_track_adapter.cc > or media_stream_video_capturer_source.cc). Note that this is > different from media conventions, where everything is piled up > in media namespace, not in an anonymous namespace either > inside or outside of the former. Ah, ok. I was just going on my admittedly foggy memories from the last time I was working within media/filters... Kind of missed that the parent was in content... Oops. :) https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:66: codec_config->kf_max_dist = 10000; On 2015/07/24 15:00:09, mcasas wrote: > On 2015/07/23 22:18:39, Tom Finegan wrote: > > note that you can end up with more keyframes than just the one (I assume you > > know this; just being pedantic) > > Hmmkay, should I add a note? Doesn't hurt to add something like, "When kf_mode is VPX_KF_AUTO libvpx will sometimes emit keyframes regardless of min/max distance out of necessity." It might prevent some confusion in the far future during a debug run. :)
On 2015/07/24 20:56:16, Tom Finegan wrote: > https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... > File content/renderer/media/video_track_recorder.cc (right): > > https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... > content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* > codec_context) { > On 2015/07/24 15:00:10, mcasas wrote: > > On 2015/07/23 22:18:39, Tom Finegan wrote: > > > A static method in an anon namespace seems unnecessary. One or the other > > should > > > suffice, no? > > > > It would but content rules "encourage" putting static functions > > and ad hoc classes in an anonymous namespace _inside_ > > content namespace (see for instance video_track_adapter.cc > > or media_stream_video_capturer_source.cc). Note that this is > > different from media conventions, where everything is piled up > > in media namespace, not in an anonymous namespace either > > inside or outside of the former. > > Ah, ok. I was just going on my admittedly foggy memories from the last time I > was working within media/filters... Kind of missed that the parent was in > content... Oops. :) > > https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... > content/renderer/media/video_track_recorder.cc:66: codec_config->kf_max_dist = > 10000; > On 2015/07/24 15:00:09, mcasas wrote: > > On 2015/07/23 22:18:39, Tom Finegan wrote: > > > note that you can end up with more keyframes than just the one (I assume you > > > know this; just being pedantic) > > > > Hmmkay, should I add a note? > > Doesn't hurt to add something like, "When kf_mode is VPX_KF_AUTO libvpx will > sometimes emit keyframes regardless of min/max distance out of necessity." It > might prevent some confusion in the far future during a debug run. :) l-g-t-m on libvpx usage. Don't want to step on any toes in content, so the real approval needs to come from someone else.
miu@ PTAL (I moved actual encoding to a background thread, PTAL) tomfinegan@: will bundle the extra comments in the next PS, thanks! https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/270001/content/renderer/media... content/renderer/media/video_track_recorder.cc:42: vpx_codec_ctx_t* codec_context) { On 2015/07/24 20:56:16, Tom Finegan wrote: > On 2015/07/24 15:00:10, mcasas wrote: > > On 2015/07/23 22:18:39, Tom Finegan wrote: > > > A static method in an anon namespace seems unnecessary. One or the other > > should > > > suffice, no? > > > > It would but content rules "encourage" putting static functions > > and ad hoc classes in an anonymous namespace _inside_ > > content namespace (see for instance video_track_adapter.cc > > or media_stream_video_capturer_source.cc). Note that this is > > different from media conventions, where everything is piled up > > in media namespace, not in an anonymous namespace either > > inside or outside of the former. > > Ah, ok. I was just going on my admittedly foggy memories from the last time I > was working within media/filters... Kind of missed that the parent was in > content... Oops. :) Acknowledged.
https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 Is this legal? You're changing a build-time config option, so the libvpx header files may no longer match the compiled libvpx binary...and crash. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:135: // Configuration happens on first frame arrival. Used on IO thread only. This won't work. MediaStreamVideoSinks must handle variable resolution. Suggest you take a look at the way we handle re-init of the encoder when the resolution changes in media/cast: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:150: base::TimeTicks timestamp_base_; How about first_frame_timestamp_? https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:176: void VideoTrackRecorder::VpxEncoder::EncodeOnIo( Ah! So the encoding doesn't actually happen on the IO thread? I'd suggest changing the naming of the "OnIo" methods. Suggestions: EncodeOnIo() --> StartFrameEncode() FinishEncodingOnIo() --> OnFrameEncodeCompleted() The point is that these are "control logic" methods, not the ones doing actual work, and that should be reflected in their names. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:183: track_index_ = on_first_frame_callback_.Run(frame->coded_size(), This must be the visible_rect().size(), not coded_size(). (Here and pretty everywhere else, FWICT.) https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:207: bool force_keyframe, You don't need to force a key frame. VP8 will automatically force a KF for the first encoded frame, and any time the frame resolution changes. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:222: // TODO(mcasas): Use VideoFrameMetadata::FRAME_DURATION if available. Quality and target bitrate will be all wrong for many video streams if you use this hard-coded duration. Please get rid of this TODO and put in the 3 LOC to grab this out of the metadata. When FRAME_DURATION is not set, you can estimate based on the change in timestamps between frames. For example: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:243: data->append(static_cast<char*>(pkt->data.frame.buf), pkt->data.frame.sz); Use assign() instead of append(). We don't want the string's underlying storage to be re-allocated under an assumption that it may grow larger. This may lead to wasting memory. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:21: // living on Render IO thread and using an encoding thread. 1. This should not be done on the IO thread. 2. Instead of naming the IO thread as the thread where things must run, why not just specify that users of this class need to provide a thread for encoding? Meaning, you can provide a task runner to the ctor (just like you are doing now), but call it encode_task_runner. Said another way: It looks like you should just change the naming for all variables and comments from "IO thread" to "encode thread." https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:28: using OnEncodedVideoCB = base::Callback<void(uint64_t track_number, |track_number| does not belong in this API nor this class. You should curry the value by binding the track number in a callback that becomes an OnEncodedVideoCB.
niklase@chromium.org changed reviewers: + niklase@chromium.org
https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media... content/renderer/media/video_track_recorder.cc:281: // prediction for the next frame's duration. Drive by shooting. This (using last frames duration as estimate of current frame duration) could create some weird artifacts if we have a camera freeze, pause, or large packet loss for remote streams. Add a TODO to look into that later?
Patchset #4 (id:330001) has been deleted
Patchset #4 (id:350001) has been deleted
miu@ PTAL https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 On 2015/07/27 22:30:09, miu wrote: > Is this legal? You're changing a build-time config option, so the libvpx header > files may no longer match the compiled libvpx binary...and crash. I guess so, is used all over the codebase [1] including remoting/codec/video_encoder_vpx.cc and media/cast/receiver/video_decoder.cc (where you landed it [2] :) -- arguably it was a refactor) [1] https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... [2] https://codereview.chromium.org/225023010 https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:135: // Configuration happens on first frame arrival. Used on IO thread only. On 2015/07/27 22:30:09, miu wrote: > This won't work. MediaStreamVideoSinks must handle variable resolution. > Suggest you take a look at the way we handle re-init of the encoder when the > resolution changes in media/cast: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... Done. Shamelessly taken over big chunks of the code there. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:150: base::TimeTicks timestamp_base_; On 2015/07/27 22:30:09, miu wrote: > How about first_frame_timestamp_? Done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:176: void VideoTrackRecorder::VpxEncoder::EncodeOnIo( On 2015/07/27 22:30:09, miu wrote: > Ah! So the encoding doesn't actually happen on the IO thread? I'd suggest > changing the naming of the "OnIo" methods. Suggestions: > > EncodeOnIo() --> StartFrameEncode() > FinishEncodingOnIo() --> OnFrameEncodeCompleted() > > The point is that these are "control logic" methods, not the ones doing actual > work, and that should be reflected in their names. Good suggestion, done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:183: track_index_ = on_first_frame_callback_.Run(frame->coded_size(), On 2015/07/27 22:30:09, miu wrote: > This must be the visible_rect().size(), not coded_size(). (Here and pretty > everywhere else, FWICT.) Done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:207: bool force_keyframe, On 2015/07/27 22:30:09, miu wrote: > You don't need to force a key frame. VP8 will automatically force a KF for the > first encoded frame, and any time the frame resolution changes. Done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:222: // TODO(mcasas): Use VideoFrameMetadata::FRAME_DURATION if available. On 2015/07/27 22:30:09, miu wrote: > Quality and target bitrate will be all wrong for many video streams if you use > this hard-coded duration. Please get rid of this TODO and put in the 3 LOC to > grab this out of the metadata. > > When FRAME_DURATION is not set, you can estimate based on the change in > timestamps between frames. For example: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... Done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:243: data->append(static_cast<char*>(pkt->data.frame.buf), pkt->data.frame.sz); On 2015/07/27 22:30:09, miu wrote: > Use assign() instead of append(). We don't want the string's underlying storage > to be re-allocated under an assumption that it may grow larger. This may lead > to wasting memory. Done. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:21: // living on Render IO thread and using an encoding thread. On 2015/07/27 22:30:09, miu wrote: > 1. This should not be done on the IO thread. > > 2. Instead of naming the IO thread as the thread where things must run, why not > just specify that users of this class need to provide a thread for encoding? > Meaning, you can provide a task runner to the ctor (just like you are doing > now), but call it encode_task_runner. Said another way: It looks like you > should just change the naming for all variables and comments from "IO thread" to > "encode thread." What I meant is that callbacks to the "internal VpxEncoder" happen on IO thread. This "internal VpxEncoder" has a worker thread inside. There are, thus, 3 threads involved in this party (!). I changed VpxEncoder to be more lenient on the threads: now it caches the VideoFrame delivery thread (which should be the IO thread) and reuses it for callback execution -- instead of getting it on constructor. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:28: using OnEncodedVideoCB = base::Callback<void(uint64_t track_number, On 2015/07/27 22:30:09, miu wrote: > |track_number| does not belong in this API nor this class. You should curry the > value by binding the track number in a callback that becomes an > OnEncodedVideoCB. I'm not sure I understand. Do you mean OnFirstFrameCB should return a OnEncodedVideoCB() with the first parameter bound, or something else? https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/310001/content/renderer/media... content/renderer/media/video_track_recorder.cc:281: // prediction for the next frame's duration. On 2015/07/30 19:46:47, Niklas Enbom wrote: > Drive by shooting. This (using last frames duration as estimate of current frame > duration) could create some weird artifacts if we have a camera freeze, pause, > or large packet loss for remote streams. Add a TODO to look into that later? Done.
https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.cc:17: #define VPX_CODEC_DISABLE_COMPAT 1 On 2015/07/31 11:26:27, mcasas wrote: > On 2015/07/27 22:30:09, miu wrote: > > Is this legal? You're changing a build-time config option, so the libvpx > header > > files may no longer match the compiled libvpx binary...and crash. > > I guess so, is used all over the codebase [1] including Weird. Well, okay I guess. ;-) https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:28: using OnEncodedVideoCB = base::Callback<void(uint64_t track_number, On 2015/07/31 11:26:27, mcasas wrote: > On 2015/07/27 22:30:09, miu wrote: > > |track_number| does not belong in this API nor this class. You should curry > the > > value by binding the track number in a callback that becomes an > > OnEncodedVideoCB. > > I'm not sure I understand. Do you mean OnFirstFrameCB should > return a OnEncodedVideoCB() with the first parameter bound, or > something else? Sorry. I wasn't very clear here. Basically, I think you're letting the details of your WebM muxer have too much influence on the interface in this class. In particular, VideoTrackRecorder doesn't care about |track_number| at all. My suggestion: Get rid of OnFirstFrameCB, and define OnEncodedVideoCB as: using OnEncodedVideoCB = base::Callback<void( const scoped_refptr<media::VideoFrame>& source_frame, const base::StringPiece& encoded_data, base::TimeTicks reference_timestamp, bool is_key_frame)>; Then, your client code would do something like: class MyClient { public: MyClient(io_task_runner, const blink::WebMediaStreamTrack& track) : webm_muxer_track_weak_factory_(this) { recorder_.reset(new VideoTrackRecorder(io_task_runner, track, base::Bind(&MyClient::OnEncodedFrame, weak_factory_.GetWeakPtr())); } private: void OnEncodedFrame(const scoped_refptr<media::VideoFrame>& source_frame, const base::StringPiece& encoded_data, base::TimeTicks reference_timestamp, bool is_key_frame) { if (!muxer_) { // This is the first frame. muxer_.reset(new WebmMuxer()); double frame_rate = 0.0; source_frame->metadata()->GetDouble(FRAME_RATE, &frame_rate); track_number_ = muxer_->AddVideoTrack(source_frame->visible_rect().size(), frame_rate); first_frame_timestamp_ = reference_timestamp; } muxer_->OnEncodedVideo(track_number_, encoded_data, reference_timestamp - first_frame_timestamp_, is_key_frame); } scoped_ptr<VideoTrackRecorder> recorder_; scoped_ptr<WebmMuxer> muxer_; uint64_t track_number_; base::TimeTicks first_frame_timestamp_; base::WeakPtrFactory<MyClient> weak_factory_; }; https://codereview.chromium.org/1233033002/diff/370001/content/renderer/media... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/1233033002/diff/370001/content/renderer/media... content/renderer/media/video_track_recorder_unittest.cc:91: const scoped_ptr<ChildProcess> child_process_; nit: Can this just be: ChildProcess child_process_;
miu@ PTAL Took in your suggestion with a twist, and that needed WebmMuxer adaptations; apologies for the delay. https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/290001/content/renderer/media... content/renderer/media/video_track_recorder.h:28: using OnEncodedVideoCB = base::Callback<void(uint64_t track_number, On 2015/08/04 04:06:30, miu wrote: > On 2015/07/31 11:26:27, mcasas wrote: > > On 2015/07/27 22:30:09, miu wrote: > > > |track_number| does not belong in this API nor this class. You should curry > > the > > > value by binding the track number in a callback that becomes an > > > OnEncodedVideoCB. > > > > I'm not sure I understand. Do you mean OnFirstFrameCB should > > return a OnEncodedVideoCB() with the first parameter bound, or > > something else? > > Sorry. I wasn't very clear here. Basically, I think you're letting the details > of your WebM muxer have too much influence on the interface in this class. In > particular, VideoTrackRecorder doesn't care about |track_number| at all. > > My suggestion: Get rid of OnFirstFrameCB, and define OnEncodedVideoCB as: > > using OnEncodedVideoCB = base::Callback<void( > const scoped_refptr<media::VideoFrame>& source_frame, > const base::StringPiece& encoded_data, > base::TimeTicks reference_timestamp, > bool is_key_frame)>; > > Then, your client code would do something like: > > class MyClient { > public: > MyClient(io_task_runner, const blink::WebMediaStreamTrack& track) > : webm_muxer_track_weak_factory_(this) { > recorder_.reset(new VideoTrackRecorder(io_task_runner, track, > base::Bind(&MyClient::OnEncodedFrame, > weak_factory_.GetWeakPtr())); > } > > private: > void OnEncodedFrame(const scoped_refptr<media::VideoFrame>& source_frame, > const base::StringPiece& encoded_data, > base::TimeTicks reference_timestamp, > bool is_key_frame) { > if (!muxer_) { // This is the first frame. > muxer_.reset(new WebmMuxer()); > double frame_rate = 0.0; > source_frame->metadata()->GetDouble(FRAME_RATE, &frame_rate); > track_number_ = muxer_->AddVideoTrack(source_frame->visible_rect().size(), > frame_rate); > first_frame_timestamp_ = reference_timestamp; > } > > muxer_->OnEncodedVideo(track_number_, encoded_data, > reference_timestamp - first_frame_timestamp_, > is_key_frame); > } > > scoped_ptr<VideoTrackRecorder> recorder_; > > scoped_ptr<WebmMuxer> muxer_; > uint64_t track_number_; > base::TimeTicks first_frame_timestamp_; > > base::WeakPtrFactory<MyClient> weak_factory_; > }; I agree that the |track_index| should not be surfaced to the client. Patchset 5 follows your advice with a twist: the OnFirstFrameCB returns and OnEncodedVideoCB with the first parameter bound, since the WebmMuxer does need this |track_index| to differentiate between multiple clients. But the index is bound where it belongs, i.e. in the CB object and not in the VideoTrackRecorder. https://codereview.chromium.org/1233033002/diff/370001/content/renderer/media... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/1233033002/diff/370001/content/renderer/media... content/renderer/media/video_track_recorder_unittest.cc:91: const scoped_ptr<ChildProcess> child_process_; On 2015/08/04 04:06:30, miu wrote: > nit: Can this just be: > > ChildProcess child_process_; Done.
https://codereview.chromium.org/1233033002/diff/390001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/390001/content/renderer/media... content/renderer/media/video_track_recorder.h:29: base::Callback<OnEncodedVideoCB(const gfx::Size& frame_size, IMO, this is even more complex than before (i.e., a callback that returns another callback). Why do we need a separate OnFirstFrameCB? I think the same argument I made for getting rid of |track_number| applies to this situation as well: In other words, VideoTrackRecorder should not need to do anything special for the first frame. That's an implementation detail for using WebmMuxer. The code for the function bound to OnEncodedVideoCB can easily check whether it's the first time the OnEncodedVideoCB is being run, and deal with that special case. (See my earlier example.) WDYT?
miu@ PTAL I followed your suggestion and moved track_index mgmt to WebmMuxer, so VideoTrackRecorder doesn't see it at all. Also, VTR sends a TimeTicks, and WebmMuxer deals with changing it to TimeStamps. The only diff with your suggestion is that WebmMuxer looks at |track_index_| to see if it has been initialized or not.
miu@ ping
Interface looks good to me. Now, just some threading, tear-down, and codec parameter items: https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:45: VpxEncoder(const OnEncodedVideoCB& on_encoded_video_callback); Need explicit keyword here. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); This class is ref-counted. Therefore, there's no guarantee this dtor will be run on the main_render_thread_. Perhaps you should remove the ref-counting? You may need to use WeakPtrs for posting tasks to the encoding_thread_. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:121: } You need to call vpx_codec_destroy() if the encoder was initialized. I'm not sure whether you'll need to either: 1. Block the current thread with a base::WaitableEvent, and hop over to the encoding_thread_ to call vpx_codec_destroy() 2. After encoding_thread_.Stop(), call vpx_codec_destroy(). #2 is much simpler, but #1 may be necessary if libvpx needs to join on the threads it creates for itself internally. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:202: // TODO(mcasas): Workaround for certain bug. What bug? ;) https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:206: //vpx_codec_destroy(&encoder_); Is this supposed to be commented out? https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:212: codec_config_.rc_target_bitrate = size.width() * size.height() * nit: size.GetArea() would be cleaner instead of width*height. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:213: codec_config_.rc_target_bitrate / IMO, I'd be a bit nervous about the default codec values changing out from underneath you. Either DCHECK() the assumptions, or define constants in this file. FWICT, this math does reasonable results based on the current libvpx defaults: for 1280x720: 1280*720*256/320/240 => 3072 for 1920x1080: 1920*1080*256/320/240 => 6912 (maybe a little high, but ok) https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:224: codec_config_.g_timebase.den = base::Time::kMillisecondsPerSecond; Microseconds please. Milliseconds is for people who want jittery playback. ;-) BTW--You're already calling vpx_encode() correctly (using duration.InMicroseconds()). https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:228: // max distance out of necessity. Due to http://crbug.com/440223, decoding Are you sure this is the same problem? If you use this to record to a webm file and then try to play the webm file in a normal <VIDEO src="..."> element, what happens? https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:232: codec_config_.kf_min_dist = 10000; If this really is needed, I'd suggest setting the min and max to a wider range and let the encoder decide where it's best to generate a KF. Suggestion: min=0, max=30000 https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:237: You may also want to set codec_config_.g_threads. Otherwise, IIRC, you get one (which on most systems probably won't be enough to keep up in realtime). Possible formula: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me... https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:267: return std::max(predicted_frame_duration, kMinFrameDuration); Suggest you upper-bound this as well. It's possible there will be minutes or hours between two frames (e.g., static screen capture, screen change after a long resting period). Something like base::TimeDelta::FromSecondsD(1.0 / 8) should work well. https://codereview.chromium.org/1233033002/diff/410001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1233033002/diff/410001/media/capture/webm_mux... media/capture/webm_muxer.h:29: // OnEncodedVideoCB() returned by AddVideoTrack(). libwebm will eventually ping Revert this comment w/ latest changes?
Patchset #7 (id:430001) has been deleted
Patchset #7 (id:450001) has been deleted
Patchset #7 (id:470001) has been deleted
Patchset #7 (id:490001) has been deleted
miu@ PTAL (tomfinegan@ please have a look at the q's) https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:45: VpxEncoder(const OnEncodedVideoCB& on_encoded_video_callback); On 2015/08/18 02:08:35, miu wrote: > Need explicit keyword here. Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/08/18 02:08:35, miu wrote: > This class is ref-counted. Therefore, there's no guarantee this dtor will be > run on the main_render_thread_. Perhaps you should remove the ref-counting? > You may need to use WeakPtrs for posting tasks to the encoding_thread_. Several references are held to VpxEncoder: 1. VideoTrackRecorder has one, on Main Render thread. 2. the VideoCaptureDeliverFrameCB in l.278 holds another 3. VpxEncoder internal PostTask()'s, one for jumping to EncodeOnEncodingThread and one for jumping back to whichever thread called it (render IO thread). I think the problem comes from 2., which is the thread we don't control and that could keep VpxEncoder alive beyond VTR and its own PostTask. So I propose a similar solution to the other MSVSinks, that is, adding an intermediate step in VTR, namely an OnVideoFrame() which will forward to VpxEncoder if some marker does not prevent that from happening. I'm using |track_.is_null()| as the marker. WDYT? (CastVideoSink [1] is such a VideoSink, uses a bool |sink_added_| as marker) (WebRtcVideoTrackAdapter has a similar approach but is more complex due to jingle interactions). [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me... https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:121: } On 2015/08/18 02:08:35, miu wrote: > You need to call vpx_codec_destroy() if the encoder was initialized. I'm not > sure whether you'll need to either: > > 1. Block the current thread with a base::WaitableEvent, and hop over to the > encoding_thread_ to call vpx_codec_destroy() > > 2. After encoding_thread_.Stop(), call vpx_codec_destroy(). > > #2 is much simpler, but #1 may be necessary if libvpx needs to join on the > threads it creates for itself internally. I've been digging down the vpx_codec_destroy() f pointers and vp8e_destroy() [1] doesn't seem to be doing anything specific about the threads, so +1 for 2. tomfinegan@ WDYT? [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx... https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:202: // TODO(mcasas): Workaround for certain bug. On 2015/08/18 02:08:35, miu wrote: > What bug? ;) Oops, sorry, both this TODO and the commented out l.206 was during experiments to get the ut working, and forgot to update them to reflect [1]. Pardon! [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:206: //vpx_codec_destroy(&encoder_); On 2015/08/18 02:08:35, miu wrote: > Is this supposed to be commented out? Acknowledged. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:212: codec_config_.rc_target_bitrate = size.width() * size.height() * On 2015/08/18 02:08:35, miu wrote: > nit: size.GetArea() would be cleaner instead of width*height. Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:213: codec_config_.rc_target_bitrate / On 2015/08/18 02:08:35, miu wrote: > IMO, I'd be a bit nervous about the default codec values changing out from > underneath you. Either DCHECK() the assumptions, or define constants in this > file. > > FWICT, this math does reasonable results based on the current libvpx defaults: > > for 1280x720: 1280*720*256/320/240 => 3072 > for 1920x1080: 1920*1080*256/320/240 => 6912 (maybe a little high, but ok) Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:224: codec_config_.g_timebase.den = base::Time::kMillisecondsPerSecond; On 2015/08/18 02:08:35, miu wrote: > Microseconds please. Milliseconds is for people who want jittery playback. ;-) > > BTW--You're already calling vpx_encode() correctly (using > duration.InMicroseconds()). Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:228: // max distance out of necessity. Due to http://crbug.com/440223, decoding On 2015/08/18 02:08:35, miu wrote: > Are you sure this is the same problem? If you use this to record to a webm file > and then try to play the webm file in a normal <VIDEO src="..."> element, what > happens? I don't know. I assumed so due to the similar configuration etc. In any case l.231-233 need to be configured, so these values looked OK to me. tomfinegan@ WDYT? https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:232: codec_config_.kf_min_dist = 10000; On 2015/08/18 02:08:35, miu wrote: > If this really is needed, I'd suggest setting the min and max to a wider range > and let the encoder decide where it's best to generate a KF. Suggestion: min=0, > max=30000 Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:237: On 2015/08/18 02:08:35, miu wrote: > You may also want to set codec_config_.g_threads. Otherwise, IIRC, you get one > (which on most systems probably won't be enough to keep up in realtime). > > Possible formula: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me... Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:267: return std::max(predicted_frame_duration, kMinFrameDuration); On 2015/08/18 02:08:35, miu wrote: > Suggest you upper-bound this as well. It's possible there will be minutes or > hours between two frames (e.g., static screen capture, screen change after a > long resting period). Something like base::TimeDelta::FromSecondsD(1.0 / 8) > should work well. Done. https://codereview.chromium.org/1233033002/diff/410001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1233033002/diff/410001/media/capture/webm_mux... media/capture/webm_muxer.h:29: // OnEncodedVideoCB() returned by AddVideoTrack(). libwebm will eventually ping On 2015/08/18 02:08:35, miu wrote: > Revert this comment w/ latest changes? Done.
I can dig more deeply into this if you two would like: I'm in a huge rush right now, so I wanted to get what I could answer quickly posted in case I was blocking one of you. Please let me know! https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:121: } On 2015/08/19 00:16:30, mcasas (slow to review) wrote: > On 2015/08/18 02:08:35, miu wrote: > > You need to call vpx_codec_destroy() if the encoder was initialized. I'm not > > sure whether you'll need to either: > > > > 1. Block the current thread with a base::WaitableEvent, and hop over to the > > encoding_thread_ to call vpx_codec_destroy() > > > > 2. After encoding_thread_.Stop(), call vpx_codec_destroy(). > > > > #2 is much simpler, but #1 may be necessary if libvpx needs to join on the > > threads it creates for itself internally. > > I've been digging down the vpx_codec_destroy() f pointers and > vp8e_destroy() [1] doesn't seem to be doing anything specific > about the threads, so +1 for 2. > > tomfinegan@ WDYT? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx... 2 is fine unless you're going to end up calling vpx_codec_destroy() while your encode thread is already waiting on a call to vpx_codec_encode() or vpx_codec_get_cx_data(), then I'm not sure-- I suspect you'd end up stuck waiting on a join when vpx_thread cleans up its workers. If your encoding thread has already been joined, and you're certain a thread isn't already waiting on the encoder, 2 is safe. Is the concern a stall while inside vpx_codec_destroy() while waiting on it to shut down? https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:228: // max distance out of necessity. Due to http://crbug.com/440223, decoding On 2015/08/19 00:16:30, mcasas (slow to review) wrote: > On 2015/08/18 02:08:35, miu wrote: > > Are you sure this is the same problem? If you use this to record to a webm > file > > and then try to play the webm file in a normal <VIDEO src="..."> element, what > > happens? > > I don't know. I assumed so due to the similar configuration etc. > In any case l.231-233 need to be configured, so these values > looked OK to me. > > tomfinegan@ WDYT? What's the question? Whether or not a file with 30000 consecutive non-keyframes will still cause the problem noted in the bug? According to the comments in the bug (#1, actually), this only effect a media source decode, so no, I don't think a <VIDEO src=...> element is going to be effected unless its hooked up for playback via MSE. I suspect the problem remains-- I'm not aware of an upstream bug. Want to file one? I didn't have time to read through everything in there: sort of in a rush... not certain if this is a bug in libvpx or within the MSE stuff. Anyway, tracker is here: https://code.google.com/p/webm/issues/list :)
Patchset #7 (id:510001) has been deleted
Patchset #7 (id:530001) has been deleted
Patchset #7 (id:550001) has been deleted
https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/08/19 00:16:30, mcasas wrote: > On 2015/08/18 02:08:35, miu wrote: > > This class is ref-counted. Therefore, there's no guarantee this dtor will be > > Several references are held to VpxEncoder: > 1. VideoTrackRecorder has one, on Main Render thread. > 2. the VideoCaptureDeliverFrameCB in l.278 holds another > 3. VpxEncoder internal PostTask()'s, one for jumping to > EncodeOnEncodingThread and one for jumping back to > whichever thread called it (render IO thread). It looks like #3 is the only remaining issue. And, you don't need ref-counting for that. See comment below. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:121: } My concern is that the thread calling vpx_codec_enc_init() and vpx_codec_encode() may cause libvpx to spawn one or more threads to speed up encoding. My understanding is that these threads remain alive between invocations of vpx_codec_encode(), as it would be silly to spawn *and* join on them for each call to vpx_codec_encode(). Thus, I'm assuming vpx_codec_destroy() is where the threads are shut down. The issue: Unless libvpx does something weird, shouldn't these encode threads be joined with the thread that originally spawned them? https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:180: this, You can get rid of ref-counting if OnFrameEncodeCompleted() is made a static method. All you need to do is also pass on_encoded_video_callback_ as an additional arg. https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:205: // size, in terms of area, the existing encoder instance _could_ continue. I don't understand. This comment is explaining something the code doesn't do here. Did you mean to grab more of the code from media/cast/sender/vp8_encoder.cc? Otherwise, this sounds more like a TODO comment. https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:315: if (!track_.isNull()) This should be DCHECK(!track_.isNull()). Or, better yet, put this DCHECK in the ctor.
https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:121: } On 2015/08/19 19:43:45, miu wrote: > My concern is that the thread calling vpx_codec_enc_init() and > vpx_codec_encode() may cause libvpx to spawn one or more threads to speed up > encoding. My understanding is that these threads remain alive between > invocations of vpx_codec_encode(), as it would be silly to spawn *and* join on > them for each call to vpx_codec_encode(). Thus, I'm assuming > vpx_codec_destroy() is where the threads are shut down. > The main thread waits for all workers to complete before returning from an encode call. There is no internal event loop running inside libvpx. vpx_codec_destroy will join _and_ destroy the threads. Note, this behavior is implemented differently in VP8 and VP9. For VP8, see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx... For VP9, see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx... > The issue: Unless libvpx does something weird, shouldn't these encode threads be > joined with the thread that originally spawned them? For VP9 Libvpx spawns workers on a per tile basis. Number of workers is tiles - 1: - main thread (yours) gives tiles - 1 tiles to workers (one to each) for processing - main thread works on remaining tile, then waits for workers to finish This is when libvpx returns from vpx_codec_encode(). The threads are idle when your main thread is not executing within the vpx_codec_encode() call. The basic rule (threads idle when not encoding) holds for VP8, although the count is not tile based and the implementation is different because of differences between the codecs... Anyway, this is going pretty far back, but iirc it's macro block row based with a sync range (basically how many rows belong to each worker, and where they must align).
miu@ PTAL https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: DCHECK(main_render_thread_checker_.CalledOnValidThread()); On 2015/08/19 19:43:45, miu wrote: > On 2015/08/19 00:16:30, mcasas wrote: > > On 2015/08/18 02:08:35, miu wrote: > > > This class is ref-counted. Therefore, there's no guarantee this dtor will > be > > > > Several references are held to VpxEncoder: > > 1. VideoTrackRecorder has one, on Main Render thread. > > 2. the VideoCaptureDeliverFrameCB in l.278 holds another > > 3. VpxEncoder internal PostTask()'s, one for jumping to > > EncodeOnEncodingThread and one for jumping back to > > whichever thread called it (render IO thread). > > It looks like #3 is the only remaining issue. And, you don't need ref-counting > for that. See comment below. Done. https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... content/renderer/media/video_track_recorder.cc:228: // max distance out of necessity. Due to http://crbug.com/440223, decoding On 2015/08/19 04:20:10, Tom Finegan wrote: > On 2015/08/19 00:16:30, mcasas (slow to review) wrote: > > On 2015/08/18 02:08:35, miu wrote: > > > Are you sure this is the same problem? If you use this to record to a webm > > file > > > and then try to play the webm file in a normal <VIDEO src="..."> element, > what > > > happens? > > > > I don't know. I assumed so due to the similar configuration etc. > > In any case l.231-233 need to be configured, so these values > > looked OK to me. > > > > tomfinegan@ WDYT? > > What's the question? Whether or not a file with 30000 consecutive non-keyframes > will still cause the problem noted in the bug? According to the comments in the > bug (#1, actually), this only effect a media source decode, so no, I don't think > a <VIDEO src=...> element is going to be effected unless its hooked up for > playback via MSE. I suspect the problem remains-- I'm not aware of an upstream > bug. Want to file one? I didn't have time to read through everything in there: > sort of in a rush... not certain if this is a bug in libvpx or within the MSE > stuff. Anyway, tracker is here: https://code.google.com/p/webm/issues/list :) Acknowledged. https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:180: this, On 2015/08/19 19:43:45, miu wrote: > You can get rid of ref-counting if OnFrameEncodeCompleted() is made a static > method. All you need to do is also pass on_encoded_video_callback_ as an > additional arg. Done. https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:205: // size, in terms of area, the existing encoder instance _could_ continue. On 2015/08/19 19:43:45, miu wrote: > I don't understand. This comment is explaining something the code doesn't do > here. Did you mean to grab more of the code from > media/cast/sender/vp8_encoder.cc? > > Otherwise, this sounds more like a TODO comment. Done. https://codereview.chromium.org/1233033002/diff/570001/content/renderer/media... content/renderer/media/video_track_recorder.cc:315: if (!track_.isNull()) On 2015/08/19 19:43:45, miu wrote: > This should be DCHECK(!track_.isNull()). Or, better yet, put this DCHECK in the > ctor. Done.
On 2015/08/19 20:04:12, Tom Finegan wrote: > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... > File content/renderer/media/video_track_recorder.cc (right): > > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... > content/renderer/media/video_track_recorder.cc:121: } > On 2015/08/19 19:43:45, miu wrote: > > My concern is that the thread calling vpx_codec_enc_init() and > > vpx_codec_encode() may cause libvpx to spawn one or more threads to speed up > > vpx_codec_destroy will join _and_ destroy the threads. Note, this behavior is > implemented differently in VP8 and VP9. Okay, so to be clear, do you think this is a valid start-up/shutdown sequence: 1. Thread A calls vpx_codec_encode() which causes the threads to spin up. 2. Thread A returns from call to vpx_codec_encode(). At this point all spawned threads are alive, but are idle. 3. Thread A ends. 4. Thread B calls vpx_codec_destroy(), which joins on the vpx threads. My concern is that Thread A has to be the one to call vpx_codec_destroy(). But, if it's valid for any thread to join on the vpx threads, so long as we know they are idle, then I think the code in this CL is fine. Note that, in Chromium, base::Thread's can only be stopped/joined by the thread that originally spawned them.
lgtm % nits, and assuming tomfinegan's response is that the call to vpx_codec_destroy() by a different thread is okay. https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... content/renderer/media/video_track_recorder.cc:59: virtual ~VpxEncoder(); nit: No need to make this virtual (and make the class final for safety). https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... content/renderer/media/video_track_recorder.cc:184: base::Bind(OnFrameEncodeCompleted, nit: &OnFrameEncodeCompleted
On 2015/08/20 21:30:57, miu wrote: > On 2015/08/19 20:04:12, Tom Finegan wrote: > > > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... > > File content/renderer/media/video_track_recorder.cc (right): > > > > > https://codereview.chromium.org/1233033002/diff/410001/content/renderer/media... > > content/renderer/media/video_track_recorder.cc:121: } > > On 2015/08/19 19:43:45, miu wrote: > > > My concern is that the thread calling vpx_codec_enc_init() and > > > vpx_codec_encode() may cause libvpx to spawn one or more threads to speed up > > > > vpx_codec_destroy will join _and_ destroy the threads. Note, this behavior is > > implemented differently in VP8 and VP9. > > Okay, so to be clear, do you think this is a valid start-up/shutdown sequence: > > 1. Thread A calls vpx_codec_encode() which causes the threads to spin up. > 2. Thread A returns from call to vpx_codec_encode(). At this point all spawned > threads are alive, but are idle. > 3. Thread A ends. > 4. Thread B calls vpx_codec_destroy(), which joins on the vpx threads. > > My concern is that Thread A has to be the one to call vpx_codec_destroy(). But, > if it's valid for any thread to join on the vpx threads, so long as we know they > are idle, then I think the code in this CL is fine. > The lib does not care. The only thing that matters is that you aren't calling into the library from multiple threads without synchronization. > Note that, in Chromium, base::Thread's can only be stopped/joined by the thread > that originally spawned them. That's a Chromium base::Thread feature.
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@ PTAL/Owners RS https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... content/renderer/media/video_track_recorder.cc:59: virtual ~VpxEncoder(); On 2015/08/20 21:31:37, miu wrote: > nit: No need to make this virtual (and make the class final for safety). Done. https://codereview.chromium.org/1233033002/diff/590001/content/renderer/media... content/renderer/media/video_track_recorder.cc:184: base::Bind(OnFrameEncodeCompleted, On 2015/08/20 21:31:37, miu wrote: > nit: &OnFrameEncodeCompleted Done.
Just skimmed, deferring to miu@ for full review, but lgtm % some nits. https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.cc:62: const base::TimeTicks& capture_timestamp); no const& for all TimeTicks. https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.cc:103: encoding_thread_("EncodingThread") { Do you want to spin up one of these for each encoder or have an external resource manage a pool? https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.h:41: const base::TimeTicks& capture_time); base::TimeTicks
Patchset #10 (id:630001) has been deleted
https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.cc:62: const base::TimeTicks& capture_timestamp); On 2015/08/25 17:02:29, DaleCurtis wrote: > no const& for all TimeTicks. Done. https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.cc:103: encoding_thread_("EncodingThread") { On 2015/08/25 17:02:29, DaleCurtis wrote: > Do you want to spin up one of these for each encoder or have an external > resource manage a pool? I'd say is easier to have one encapsulated here, and I see as the overwhelming majority of use cases involving one video track being encoded at a give time at most. We can see the UMAs. https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1233033002/diff/610001/content/renderer/media... content/renderer/media/video_track_recorder.h:41: const base::TimeTicks& capture_time); On 2015/08/25 17:02:29, DaleCurtis wrote: > base::TimeTicks Argh! Done where I could, but this signature cannot be changed: is plugged into MediaStreamVideoTrack as a VideoCaptureDeliverFrameCB, defined in [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c...
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/1233033002/#ps650001 (title: "dalecurtis@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233033002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233033002/650001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mcasas@chromium.org changed reviewers: + avi@chromium.org
avi@ can you LGTM content/renderer/media/DEPS ? Added third_party/libvpx.
mcasas@chromium.org changed reviewers: - avi@chromium.org
lgtm
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/1233033002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233033002/650001
avi@chromium.org changed reviewers: + avi@chromium.org
LGTM
Message was sent while issue was closed.
Committed patchset #10 (id:650001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a466bd2cc32a9d5bcba758473ebd993a296f7d6d Cr-Commit-Position: refs/heads/master@{#345437} |