Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1990643002:
Add VEAEncoder to VideoTrackRecorder (Closed)
|
Created:
4 years, 7 months ago by emircan Modified:
4 years, 7 months ago Reviewers:
mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd VEAEncoder to VideoTrackRecorder
This CL introduces VEAEncoder which is responsible for handling
GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder
prioritizes VEAEncoders before SW implementations.
This CL is [2 of 2] to add support for Video Encode Accelerator in
VideoTrackRecorder.
Playground CL: https://codereview.chromium.org/1934093002/
BUG=608385
TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html
Committed: https://crrev.com/582f00447d5ec6830b202304429f552904f813df
Cr-Commit-Position: refs/heads/master@{#395464}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Fix unittests. #
Total comments: 34
Patch Set 3 : mcasas@ comments. #
Total comments: 18
Patch Set 4 : mcasas@ comments. #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== new code BUG= ========== to ========== Add VEAEncoder to VideoTrackRecorder This CL introduces VEAEncoder which is responsible for handling GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder prioritizes VEAEncoders before SW implementations. This CL is [2 of 2] to add support for Video Encode Accelerator in VideoTrackRecorder. Playground CL: https://codereview.chromium.org/1934093002/ BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTAL.
Uploaded a new patch to address unittests.
Just wanted to send a comment upfront and will continue review on PS2. https://codereview.chromium.org/1990643002/diff/40001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/40001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:942: } else { Maybe we can still change this |else| to if (!encoder) { so if l.940 fails for any reason, we have a sw fallback path ready? Ah but l.940 is a constructor, so it can't fail. What if RenderThreadImpl::current() or RenderThreadImpl::current()->GetGpuFactories() are not defined, like in some tests? We can inject those and/or test them before l.940. Just an idea.
https://codereview.chromium.org/1990643002/diff/40001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/40001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:942: } else { On 2016/05/18 21:36:09, mcasas wrote: > Maybe we can still change this |else| to > if (!encoder) { > so if l.940 fails for any reason, we have > a sw fallback path ready? Ah but l.940 is > a constructor, so it can't fail. What if > RenderThreadImpl::current() > or > RenderThreadImpl::current()->GetGpuFactories() > are not defined, like in some tests? We > can inject those and/or test them before > l.940. Just an idea. In PS#2, when RenderThreadImpl::current() is not defined, GetVEASupportedProfile returns VIDEO_CODEC_PROFILE_UNKNOWN and it falls to SW.
A few comments/ideas to get you going. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:42: namespace { IIRC, you can move this anonymous namespace inside content namespace (which would also spare some content:: resolutions in the function below). See e.g. [1]. Note that media/, for obscure reasons, doesn't seem to follow this pattern, and that's probably what confused you. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:50: media::VideoCodecProfile GetVEASupportedProfile( nit: s/GetVEASupportedProfile/CodecIdToVEAProfile/ ? if so, update the comment in regards to "preferred", since it'd just a translation, not a prioritization. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:78: return media::VIDEO_CODEC_PROFILE_UNKNOWN; Just some side nit, if you could refactor this block into being data-based, i.e. operating on something like: static struct { VideoTrackRecorder::CodecId codec_id; media::VideoCodecProfile min_profile; media::VideoCodecProfile max_profile; } const kSupportedVideoCodecIdToProfile = { {VideoTrackRecorder::CodecId::VP8, media::VP8PROFILE_MIN, media::VP8PROFILE_MAX}, {VideoTrackRecorder::CodecId::VP9, media::VP9PROFILE_MIN, media::VP9PROFILE_MAX}, {VideoTrackRecorder::CodecId::H264, media::H264PROFILE_MIN, media::H264PROFILE_MAX}}; https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:229: class VEAEncoder final : public VideoTrackRecorder::Encoder, Could you comment on the threading here plz? I see that most of the method are under DCHECK(encoding_task_runner_->BelongsToCurrentThread()); https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:250: // VideoTrackRecorder::Encoder ... implementation and that should only apply to the methods under the destructor, right? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:258: media::VideoCodecProfile codec_; const? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:264: ScopedVector<base::SharedMemory> output_buffers_; ScopedVector is deprecated [1], instead, use std::vector<<std::unique_ptr<>>, which will in turn not support push_back. (also note that emplace_back is supported now - http://chromium-cpp.appspot.com/). [1] https://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-types-o... https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:276: std::unique_ptr<std::pair<scoped_refptr<VideoFrame>, base::TimeTicks>> I suggest using VideoFrameAndTimestamp = std::pair<scoped_refptr<VideoFrame>, base::TimeTicks>; :) https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:397: void VEAEncoder::RequireBitstreamBuffers(unsigned int input_count, Shouldn't we at least DCHECK_GE(kVEAEncoderOutputBufferCount, input_count); ? (Unused params are commented out /* */) https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:409: DCHECK(gpu_factories_); Needed? It's already checked in ctor l.388? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:451: error_notified_ = true; It's unfortunate we don't have a callback of sorts to indicate that something has gone wrong. Instead it seems that |error_notified_| will prevent further frame encoding. What about adding a TODO/crbug next to this here? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:483: first_frame_.reset( So, if we hypothetically were to receive a few |frame|s before |video_encoder_| was received, we would overwrite |first_frame_|, would that be intended? I think so, otherwise we might be holding up the Real-Time flow. Perhaps we could add here a comment and/or check that, e.g. the resolution has not changed. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:489: DVLOG(3) << "An error occurred in VEA encoder"; Would it be a good idea to destroy the |video_encoder_| and try recreating it after error? (Didn't we have a CL with that idea, precisely, once?) https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:503: EncodeOnEncodingTaskRunner(first_frame->first, first_frame->second); I don't understand, we recursively call ourselves synchronously...? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:504: first_frame_encoded_ = true; Couldn't we drop |first_frame_encoded_| and instead check for |first_frame_.get()| itself? https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:524: input_buffers_.pop(); Suggestion of alternative for l.516-524 ? std::unique_ptr<base::SharedMemory> input_buffer; if (input_buffers_.empty()) { input_buffer = gpu_factories_->CreateSharedMemory(media::VideoFrame::AllocationSize( media::PIXEL_FORMAT_I420, vea_requested_input_size_)); } else { input_buffer = std::move(input_buffers_.front()); input_buffers_.pop(); } https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:573: if (!video_encoder_->Initialize(media::PIXEL_FORMAT_I420, input_size_, Bundle these two if()s ?
https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:42: namespace { On 2016/05/18 22:22:58, mcasas wrote: > IIRC, you can move this anonymous namespace > inside content namespace (which would also > spare some content:: resolutions in the function > below). See e.g. [1]. Note that media/, for obscure > reasons, doesn't seem to follow this pattern, and > that's probably what confused you. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:50: media::VideoCodecProfile GetVEASupportedProfile( On 2016/05/18 22:22:58, mcasas wrote: > nit: s/GetVEASupportedProfile/CodecIdToVEAProfile/ ? > if so, update the comment in regards to "preferred", since > it'd just a translation, not a prioritization. Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:78: return media::VIDEO_CODEC_PROFILE_UNKNOWN; On 2016/05/18 22:22:58, mcasas wrote: > Just some side nit, if you could refactor this block into > being data-based, i.e. operating on something like: > > static struct { > VideoTrackRecorder::CodecId codec_id; > media::VideoCodecProfile min_profile; > media::VideoCodecProfile max_profile; > } > const kSupportedVideoCodecIdToProfile = { > {VideoTrackRecorder::CodecId::VP8, media::VP8PROFILE_MIN, > media::VP8PROFILE_MAX}, > {VideoTrackRecorder::CodecId::VP9, media::VP9PROFILE_MIN, > media::VP9PROFILE_MAX}, > {VideoTrackRecorder::CodecId::H264, media::H264PROFILE_MIN, > media::H264PROFILE_MAX}}; Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:229: class VEAEncoder final : public VideoTrackRecorder::Encoder, On 2016/05/18 22:22:58, mcasas wrote: > Could you comment on the threading here plz? > I see that most of the method are under > DCHECK(encoding_task_runner_->BelongsToCurrentThread()); Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:250: // VideoTrackRecorder::Encoder On 2016/05/18 22:22:58, mcasas wrote: > ... implementation > and that should only apply to the methods > under the destructor, right? See l.303 and 355. I think it should apply to dtor as well since it is an override. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:258: media::VideoCodecProfile codec_; On 2016/05/18 22:22:58, mcasas wrote: > const? Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:264: ScopedVector<base::SharedMemory> output_buffers_; On 2016/05/18 22:22:58, mcasas wrote: > ScopedVector is deprecated [1], instead, use > std::vector<<std::unique_ptr<>>, which will > in turn not support push_back. (also note > that emplace_back is supported now - > http://chromium-cpp.appspot.com/). > > [1] > https://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-types-o... Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:276: std::unique_ptr<std::pair<scoped_refptr<VideoFrame>, base::TimeTicks>> On 2016/05/18 22:22:58, mcasas wrote: > I suggest > > using VideoFrameAndTimestamp > = std::pair<scoped_refptr<VideoFrame>, base::TimeTicks>; > > :) Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:397: void VEAEncoder::RequireBitstreamBuffers(unsigned int input_count, On 2016/05/18 22:22:58, mcasas wrote: > Shouldn't we at least > DCHECK_GE(kVEAEncoderOutputBufferCount, input_count); > ? > > (Unused params are commented out /* */) Not really as those two aren't related. I am commenting it out. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:409: DCHECK(gpu_factories_); On 2016/05/18 22:22:58, mcasas wrote: > Needed? It's already checked in ctor l.388? Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:451: error_notified_ = true; On 2016/05/18 22:22:58, mcasas wrote: > It's unfortunate we don't have a callback > of sorts to indicate that something has > gone wrong. Instead it seems that > |error_notified_| will prevent further > frame encoding. What about adding a > TODO/crbug next to this here? Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:483: first_frame_.reset( On 2016/05/18 22:22:58, mcasas wrote: > So, if we hypothetically were to receive > a few |frame|s before |video_encoder_| was > received, we would overwrite |first_frame_|, > would that be intended? I think so, otherwise > we might be holding up the Real-Time flow. > Perhaps we could add here a comment and/or > check that, e.g. the resolution has not changed. Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:489: DVLOG(3) << "An error occurred in VEA encoder"; On 2016/05/18 22:22:58, mcasas wrote: > Would it be a good idea to destroy the > |video_encoder_| and try recreating it > after error? (Didn't we have a CL with > that idea, precisely, once?) We had that logic in place for decoder. I will leave a TODO and investigate. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:503: EncodeOnEncodingTaskRunner(first_frame->first, first_frame->second); On 2016/05/18 22:22:58, mcasas wrote: > I don't understand, we recursively call > ourselves synchronously...? Second time around it wouldn't fall here since |first_frame_| is released. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:504: first_frame_encoded_ = true; On 2016/05/18 22:22:58, mcasas wrote: > Couldn't we drop |first_frame_encoded_| and > instead check for |first_frame_.get()| itself? Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:524: input_buffers_.pop(); On 2016/05/18 22:22:58, mcasas wrote: > Suggestion of alternative for l.516-524 ? > > std::unique_ptr<base::SharedMemory> input_buffer; > if (input_buffers_.empty()) { > input_buffer = > gpu_factories_->CreateSharedMemory(media::VideoFrame::AllocationSize( > media::PIXEL_FORMAT_I420, vea_requested_input_size_)); > } else { > input_buffer = std::move(input_buffers_.front()); > input_buffers_.pop(); > } Done. https://codereview.chromium.org/1990643002/diff/60001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:573: if (!video_encoder_->Initialize(media::PIXEL_FORMAT_I420, input_size_, On 2016/05/18 22:22:58, mcasas wrote: > Bundle these two if()s ? Done.
Patchset #3 (id:80001) has been deleted
Almost there! Just a few nits/questions/suggestions. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:263: Remove extra empty line. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:267: // VideoTrackRecorder::Encoder nit: still missing "implementation", i.e. // VideoTrackRecorder::Encoder implementation. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:426: } Micro-nit: I wonder how often this NOTREACHED() gets hit in developer builds, IOW, I guess CreateSharedMemory would fail to allocate if the GPU process is dead, or there is memory pressure... IF we were to kick it, we could leave these lines as if (shm) output_buffers_.push_back(base::WrapUnique(shm.release())); which read better :) https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:432: i, output_buffers_[i]->handle(), output_buffers_[i]->mapped_size())); for (size_t i = 0; i < output_buffers_.size(); ++i) UseOutputBitstreamBufferId(i); ? https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:498: frame, capture_timestamp)); std::make_pair(frame, capture_timestamp) https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:507: if (output_buffers_free_count_ == 0) { nit: Suggest changing this counter to a boolean |is_there_free_output_buffers_| or so. It confused me that it counted up and didn't count down :) https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:515: std::unique_ptr<VideoFrameAndTimestamp> first_frame(first_frame_.release()); micronit: std::unique_ptr<VideoFrameAndTimestamp> first_frame = std::move(first_frame_); ? https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:543: } This while(input_buffer-too-small) search only applies to l.533-535, right? Since in the if{} branch we allocate to our desires. Also, don't you have to check that |input_buffer| is not nullptr after the while? Suggestion for l.529-543: const int desired_mapped_size = media::VideoFrame::AllocationSize( media::PIXEL_FORMAT_I420, vea_requested_input_size_); if (input_buffers_.empty()) { input_buffer = gpu_factories_->CreateSharedMemory(desired_mapped_size); } else { do { input_buffer = std::move(input_buffers_); input_buffers_.pop(); } while (!input_buffers.empty() && input_buffer->mapped_size() < desired_mapped_size); if (!input_buffer) return; } https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:547: gfx::Rect(input_size_), input_size_, s/input_size_/vea_requested_input_size_/ in the two references of this line?
https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:263: On 2016/05/21 01:36:58, mcasas wrote: > Remove extra empty line. Done. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:267: // VideoTrackRecorder::Encoder On 2016/05/21 01:36:58, mcasas wrote: > nit: still missing "implementation", i.e. > // VideoTrackRecorder::Encoder implementation. Done. Changed it here and l.316, 368. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:426: } On 2016/05/21 01:36:57, mcasas wrote: > Micro-nit: I wonder how often this NOTREACHED() gets > hit in developer builds, IOW, I guess CreateSharedMemory > would fail to allocate if the GPU process is dead, or there > is memory pressure... > IF we were to kick it, we could leave these lines as > if (shm) > output_buffers_.push_back(base::WrapUnique(shm.release())); > > which read better :) Done. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:432: i, output_buffers_[i]->handle(), output_buffers_[i]->mapped_size())); On 2016/05/21 01:36:57, mcasas wrote: > for (size_t i = 0; i < output_buffers_.size(); ++i) > UseOutputBitstreamBufferId(i); > > ? Done. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:498: frame, capture_timestamp)); On 2016/05/21 01:36:57, mcasas wrote: > std::make_pair(frame, capture_timestamp) Implicit types cannot be decided with make_pair() since |first_frame_| is a ptr. See https://code.google.com/p/chromium/codesearch#search/&q=%22new%20std::pair%22... for usage like this. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:507: if (output_buffers_free_count_ == 0) { On 2016/05/21 01:36:57, mcasas wrote: > nit: Suggest changing this counter to a boolean > |is_there_free_output_buffers_| or so. It > confused me that it counted up and didn't > count down :) It counts down in l.443. I followed the pattern from RTCVideoEncoder https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m.... However, looking back, that pattern doesn't make much sense as we copy out the buffers in BitStreamBufferReady, so it will be put back immediately. I can only check for output_buffers_.empty() here. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:515: std::unique_ptr<VideoFrameAndTimestamp> first_frame(first_frame_.release()); On 2016/05/21 01:36:57, mcasas wrote: > micronit: > std::unique_ptr<VideoFrameAndTimestamp> first_frame = std::move(first_frame_); > ? I think release() makes it more explicit that we are giving away ownership and |first_frame_| will be null after the call. https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:543: } On 2016/05/21 01:36:57, mcasas wrote: > This while(input_buffer-too-small) search only > applies to l.533-535, right? Since in the if{} > branch we allocate to our desires. Also, don't > you have to check that |input_buffer| is not > nullptr after the while? Suggestion for l.529-543: > > const int desired_mapped_size = media::VideoFrame::AllocationSize( > media::PIXEL_FORMAT_I420, vea_requested_input_size_); > if (input_buffers_.empty()) { > input_buffer = gpu_factories_->CreateSharedMemory(desired_mapped_size); > } else { > do { > input_buffer = std::move(input_buffers_); > input_buffers_.pop(); > } while (!input_buffers.empty() && > input_buffer->mapped_size() < desired_mapped_size); > if (!input_buffer) > return; > } Yes, it applies to the else. I was thinking of a complex case here. Suppose two frames with size A are encoded and sent back. Their shm handles would be pushed back into |input_buffers| when they go out of scope via VEAEncoder::FrameFinished. Suppose the encode size changes from A to B in the meantime. Then, we need to clean out these wrong sized -A- buffers. I like your approach, but it has one problem. Suppose we pop the last input_buffer which is bad to use. !input_buffers.empty() would become false and take us out of the loop. There we still need to check if(!input_buffer || input_buffer->mapped_size() < desired_mapped_size). https://codereview.chromium.org/1990643002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:547: gfx::Rect(input_size_), input_size_, On 2016/05/21 01:36:57, mcasas wrote: > s/input_size_/vea_requested_input_size_/ in > the two references of this line? RequireBitstreamBuffers() gives us coded_size()[0]. visible_rect() should still be based on the actual frame input size here. [0] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...
Patchset #4 (id:120001) has been deleted
LGTM with one consideration for an immediate follow-up: currently there are content_browsertests testing MediaRecorder functionality, and if they all begin using the new path, we'd leave untested code -- or, if the new code is never exercised due to hw limitations, we won't test it either. Can we guarantee all paths are tested in way or another (i.e. large bots running on hardware, or via some flagging mechanism like in [1])? If flagging is the solution, let's make a bug for it, happy to take it up. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1990643002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990643002/140001
On 2016/05/23 21:51:50, mcasas wrote: > LGTM with one consideration for an immediate follow-up: > currently there are content_browsertests testing > MediaRecorder functionality, and if they all begin using > the new path, we'd leave untested code -- or, if the new > code is never exercised due to hw limitations, we won't > test it either. Can we guarantee all paths are tested > in way or another (i.e. large bots running on hardware, > or via some flagging mechanism like in [1])? If flagging > is the solution, let's make a bug for it, happy to take > it up. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med... One thing I realized is that content_browsertests only triggers VPX. For instance, H264 doesn't get triggered on Mac, SW or HW. We need to look at that issue as well.
Message was sent while issue was closed.
Description was changed from ========== Add VEAEncoder to VideoTrackRecorder This CL introduces VEAEncoder which is responsible for handling GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder prioritizes VEAEncoders before SW implementations. This CL is [2 of 2] to add support for Video Encode Accelerator in VideoTrackRecorder. Playground CL: https://codereview.chromium.org/1934093002/ BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ========== to ========== Add VEAEncoder to VideoTrackRecorder This CL introduces VEAEncoder which is responsible for handling GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder prioritizes VEAEncoders before SW implementations. This CL is [2 of 2] to add support for Video Encode Accelerator in VideoTrackRecorder. Playground CL: https://codereview.chromium.org/1934093002/ BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add VEAEncoder to VideoTrackRecorder This CL introduces VEAEncoder which is responsible for handling GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder prioritizes VEAEncoders before SW implementations. This CL is [2 of 2] to add support for Video Encode Accelerator in VideoTrackRecorder. Playground CL: https://codereview.chromium.org/1934093002/ BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html ========== to ========== Add VEAEncoder to VideoTrackRecorder This CL introduces VEAEncoder which is responsible for handling GPU encoder interactions for VideoTrackRecorder. VideoTrackRecorder prioritizes VEAEncoders before SW implementations. This CL is [2 of 2] to add support for Video Encode Accelerator in VideoTrackRecorder. Playground CL: https://codereview.chromium.org/1934093002/ BUG=608385 TEST=https://cdn.rawgit.com/miguelao/demos/master/mediarecorder.html Committed: https://crrev.com/582f00447d5ec6830b202304429f552904f813df Cr-Commit-Position: refs/heads/master@{#395464} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/582f00447d5ec6830b202304429f552904f813df Cr-Commit-Position: refs/heads/master@{#395464} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
