Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884383002:
VideoTrackRecorder: Adding inner class H264Encoder (Closed)
|
Created:
4 years, 8 months ago by mcasas Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVideoTrackRecorder: Adding inner class H264Encoder
H264 Encoder using third_party OpenH264 library. This
CL adds a new class H264Encoder <is a> Encoder, where
on ToT, there is only VpxEncoder <is a> Encoder. The
new encoder is behing #if BUILDFLAG(RTC_USE_H264).
Some code shifting from VpxEncoder to Encoder takes
place, in particular, some common functionality is
extracted from VpxEncoder into Encoder:
- StartFrameEncode() and set_paused() are extracted
and the latter is made thread safe, then renamed to
SetPaused()
- Members |main_task_runner_|, |origin_task_runner_|
and |encoding_thread_| are also extracted.
A few minor renames:
- ConfigureEncoding() --> ConfigureEncoderOnEncodingThread()
- CalculateFrameDuration() --> EstimateFrameDuration()
This is the 3rd step in a series of CLs to add support for
H264 to MediaRecorder (see https://crrev.com/1886123002/).
Next step is to MediaRecorderHandler and Blink support.
BUG=601636
TEST= see playground CL, also added unittest.
Committed: https://crrev.com/240fa42bc3097277407a3d747bcd26a33e248efa
Cr-Commit-Position: refs/heads/master@{#388965}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : emircan@ comments #
Total comments: 20
Patch Set 3 : hbos@ comments and added direct content.gyp:content_renderer to openh264 #
Total comments: 4
Patch Set 4 : hbos@ comments: adding explicit dep to openh264 in content.gyp and content/renderer/BUILD.gn #
Messages
Total messages: 32 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== VideoTrackRecorder: Adding inner class H264Encoder -- WIP WIP !! Needs/includes https://crrev.com/1893493003/ H264 Encoder using third_party OpenH264 library. This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 ========== to ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 ========== to ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This CL adds a new class H264Encoder <is a> Encoder, where on ToT, there is only VpxEncoder <is a> Encoder. The new encoder is behing #if BUILDFLAG(RTC_USE_H264). Some code shifting from VpxEncoder to Encoder takes place, in particular, some common functionality is extracted from VpxEncoder into Encoder: - StartFrameEncode() and set_paused() are extracted and the latter is made thread safe, then renamed to SetPaused() - Members |main_task_runner_|, |origin_task_runner_| and |encoding_thread_| are also extracted. A few minor renames: - ConfigureEncoding() --> ConfigureEncoderOnEncodingThread() - CalculateFrameDuration() --> EstimateFrameDuration() This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 TEST= see playground CL, also added unittest. ==========
Patchset #2 (id:80001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org, miu@chromium.org
emircan@, miu@ PTAL
Patchset #1 (id:60001) has been deleted
ping emircan@ (and miu@)
mcasas@chromium.org changed reviewers: + hbos@chromium.org
hbos@ can you PTAL at the H264Encoder internal configuration and usage? Thanks.
https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: FROM_HERE, base::Bind(&Encoder::SetPaused, this, paused)); return; https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:213: ScopedISVCEncoderPtr encoder); Can we make this a (non-static) virtual method and call in ~Encoder()? https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:498: frame = media::WrapAsI420VideoFrame(video_frame); Move l.489-498 and l.274-283 into VideoTrackRecorder::Encoder::StartFrameEncode(). https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:552: void H264Encoder::ConfigureEncoderOnEncodingThread(const gfx::Size& size) { DCHECK(encoding_thread_->task_runner()->BelongsToCurrentThread());
PTAL https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:118: FROM_HERE, base::Bind(&Encoder::SetPaused, this, paused)); On 2016/04/19 23:21:22, emircan wrote: > return; Done. https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:213: ScopedISVCEncoderPtr encoder); On 2016/04/19 23:21:21, emircan wrote: > Can we make this a (non-static) virtual method and call in ~Encoder()? I'd love to but can't because in VpxEncoder class we use a ScopedVpxCodecCtxPtr and here we use a ScopedISVCEncoderPtr. https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:498: frame = media::WrapAsI420VideoFrame(video_frame); On 2016/04/19 23:21:21, emircan wrote: > Move l.489-498 and l.274-283 into > VideoTrackRecorder::Encoder::StartFrameEncode(). Done. https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:552: void H264Encoder::ConfigureEncoderOnEncodingThread(const gfx::Size& size) { On 2016/04/19 23:21:22, emircan wrote: > DCHECK(encoding_thread_->task_runner()->BelongsToCurrentThread()); Done.
lgtm https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:213: ScopedISVCEncoderPtr encoder); On 2016/04/20 00:37:12, mcasas wrote: > On 2016/04/19 23:21:21, emircan wrote: > > Can we make this a (non-static) virtual method and call in ~Encoder()? > > I'd love to but can't because in VpxEncoder > class we use a ScopedVpxCodecCtxPtr and here > we use a ScopedISVCEncoderPtr. Ok, sounds good.
Looking good. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:24: #endif // #if BUILDFLAG(RTC_USE_H264) content_renderer now has a direct dependency on third_party/openh264.gyp:openh264_encoder (gyp) and third_party/openh264:encoder (GN). It is indirectly resolved because content_renderer depends on a bunch of webrtc stuff, some of which depend on the openh264 encoder. It would be nice if the direct dependency is listed as a dependency if 'rtc_use_h264==1' (buildflag defined in third_party/webrtc/build/common.gypi and //third_party/webrtc/build/webrtc.gni, see common_features target as an example of a chromium build file including it). If it's not too much of a hassle, sometimes GYP is completely borked. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:46: // - receives VideoFrames on |origin_task_runner_| and ruyns OnEncodedVideoCB on nit: ruyns = runs https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:217: WelsDestroySVCEncoder(codec); This runs when ~H264Encoder runs, which is ref counted? It could run when VideoTrackRecorder is destroyed. This would be the main renderer thread? Not the encoding thread? (Thread safety concern.) I don't think this is a problem though, supposedly if there is encoding still going on the encoder is still referenced and protected against destruction. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:507: picture.pData[2] = frame->data(VideoFrame::kVPlane); How does this work with visible_rect() that is not aligned from (0,0)? Or the strides if (width,height) is not 100% of the image? https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:522: // The following DCHECKs make sure that the header of each NAl is OK. nit: NAL or NAL unit https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:533: } (The NAL start codes should be part of the data and are necessary for decoding. In the webrtc repo they are stripped before being transmitted and then reinserted on the other end or something, packetization magic, not sure why.) https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:549: openh264_encoder_.reset(temp_encoder); DCHECK that the encoder is not already set (or delete the old one) to make sure we don't leak an existing encoder instance. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:580: init_params.iMultipleThreadIdc = GetNumberOfThreadsForEncoding(); I had to limit the number of threads to 1 due to unresolved issue: crbug.com/583348. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.h:13: #include "content/public/common/features.h" (FYI this is a generated header file so any target with sources including this need to depend on content/content.gyp:common_features (gyp) and //content/public/common:features (GN) or you could get flakey compilation in some cases. Though this target already depends on it so it's good.)
Patchset #3 (id:140001) has been deleted
hbos@ PTAL, particularly at content.gyp mod. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:24: #endif // #if BUILDFLAG(RTC_USE_H264) On 2016/04/20 20:28:41, hbos_chromium wrote: > content_renderer now has a direct dependency on > third_party/openh264.gyp:openh264_encoder (gyp) and third_party/openh264:encoder > (GN). It is indirectly resolved because content_renderer depends on a bunch of > webrtc stuff, some of which depend on the openh264 encoder. > > It would be nice if the direct dependency is listed as a dependency if > 'rtc_use_h264==1' (buildflag defined in third_party/webrtc/build/common.gypi and > //third_party/webrtc/build/webrtc.gni, see common_features target as an example > of a chromium build file including it). If it's not too much of a hassle, > sometimes GYP is completely borked. Added a direct dependency in the conditions section of content.gyp:content_renderer. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:46: // - receives VideoFrames on |origin_task_runner_| and ruyns OnEncodedVideoCB on On 2016/04/20 20:28:42, hbos_chromium wrote: > nit: ruyns = runs Done. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:217: WelsDestroySVCEncoder(codec); On 2016/04/20 20:28:41, hbos_chromium wrote: > This runs when ~H264Encoder runs, which is ref counted? It could run when > VideoTrackRecorder is destroyed. This would be the main renderer thread? Not the > encoding thread? (Thread safety concern.) I don't think this is a problem > though, supposedly if there is encoding still going on the encoder is still > referenced and protected against destruction. It runs on ShutdownEncoder(), posted from ~H264Encoder, but also when |encoder_| is destroyed because of a change in encoding parameters, in l.549, ConfigureEncoderOnEncodingThread(). In both cases this is only executed on |encoding_thread_|. FTR, this sequence has been polished already for the VPx encoder, where the thread hopping was very carefully reviewed by miu@. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:507: picture.pData[2] = frame->data(VideoFrame::kVPlane); On 2016/04/20 20:28:41, hbos_chromium wrote: > How does this work with visible_rect() that is not aligned from (0,0)? Or the > strides if (width,height) is not 100% of the image? Oh you're right. Corrected. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:522: // The following DCHECKs make sure that the header of each NAl is OK. On 2016/04/20 20:28:41, hbos_chromium wrote: > nit: NAL or NAL unit Done. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:533: } On 2016/04/20 20:28:42, hbos_chromium wrote: > (The NAL start codes should be part of the data and are necessary for decoding. > In the webrtc repo they are stripped before being transmitted and then > reinserted on the other end or something, packetization magic, not sure why.) Acknowledged. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:549: openh264_encoder_.reset(temp_encoder); On 2016/04/20 20:28:41, hbos_chromium wrote: > DCHECK that the encoder is not already set (or delete the old one) to make sure > we don't leak an existing encoder instance. See my comments in ISVCEncoderDeleter, the idea is that we use a ScopedISVCEncoderPtr with the appropriate semantics for its destruction. This is the same pattern used for Vpx encoder in l.350 (of this patch) using ScopedVpxCodecCtxPtr. FTR, while writing this code and before adding the specific destructor, I just used a plan unique_ptr and openh264 was correctly calling Uninitialize() and then WelsDestroySVCEncoder(). Go figure :) https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:580: init_params.iMultipleThreadIdc = GetNumberOfThreadsForEncoding(); On 2016/04/20 20:28:41, hbos_chromium wrote: > I had to limit the number of threads to 1 due to unresolved issue: > crbug.com/583348. Added comment, done. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.h (right): https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.h:13: #include "content/public/common/features.h" On 2016/04/20 20:28:42, hbos_chromium wrote: > (FYI this is a generated header file so any target with sources including this > need to depend on content/content.gyp:common_features (gyp) and > //content/public/common:features (GN) or you could get flakey compilation in > some cases. Though this target already depends on it so it's good.) Acknowledged.
Awesome, but you forgot GN, and there's one gyp change necessary. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:217: WelsDestroySVCEncoder(codec); On 2016/04/20 23:20:34, mcasas wrote: > On 2016/04/20 20:28:41, hbos_chromium wrote: > > This runs when ~H264Encoder runs, which is ref counted? It could run when > > VideoTrackRecorder is destroyed. This would be the main renderer thread? Not > the > > encoding thread? (Thread safety concern.) I don't think this is a problem > > though, supposedly if there is encoding still going on the encoder is still > > referenced and protected against destruction. > > It runs on ShutdownEncoder(), posted from ~H264Encoder, > but also when |encoder_| is destroyed because of a > change in encoding parameters, in l.549, > ConfigureEncoderOnEncodingThread(). In both cases this > is only executed on |encoding_thread_|. > > FTR, this sequence has been polished already for the > VPx encoder, where the thread hopping was very carefully > reviewed by miu@. Acknowledged. https://codereview.chromium.org/1884383002/diff/120001/content/renderer/media... content/renderer/media/video_track_recorder.cc:549: openh264_encoder_.reset(temp_encoder); On 2016/04/20 23:20:34, mcasas wrote: > On 2016/04/20 20:28:41, hbos_chromium wrote: > > DCHECK that the encoder is not already set (or delete the old one) to make > sure > > we don't leak an existing encoder instance. > > See my comments in ISVCEncoderDeleter, the idea > is that we use a ScopedISVCEncoderPtr with the > appropriate semantics for its destruction. This > is the same pattern used for Vpx encoder in > l.350 (of this patch) using ScopedVpxCodecCtxPtr. > > FTR, while writing this code and before adding the > specific destructor, I just used a plan unique_ptr > and openh264 was correctly calling Uninitialize() > and then WelsDestroySVCEncoder(). Go figure :) Oh right, if there is an old one it is appropriately destroyed. No need to DCHECK. https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp#ne... content/content.gyp:286: 'target_name': 'content_renderer', Nice, but you forgot GN. Make the same changes for the GN version. GN sample usage of rtc_use_h264: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp#ne... content/content.gyp:337: 'target_name': 'content', You should make a similar change here, when component != static_library the content_* targets are merged into a big content target, and in that case building content_renderer or any of the smaller targets means building all of content. So the target is basically defined twice, as a small blob and as a large blob, and changes have to be made in two places.
Patchset #4 (id:180001) has been deleted
hbos@ PTAL https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp#ne... content/content.gyp:286: 'target_name': 'content_renderer', On 2016/04/21 07:36:09, hbos_chromium wrote: > Nice, but you forgot GN. Make the same changes for the GN version. > > GN sample usage of rtc_use_h264: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... Done. Note that video_track_encoder.* is bundled in the .gypi private_renderer_webrtc_sources and is also sandwiched in an if(enabled_webrtc). https://codereview.chromium.org/1884383002/diff/160001/content/content.gyp#ne... content/content.gyp:337: 'target_name': 'content', On 2016/04/21 07:36:09, hbos_chromium wrote: > You should make a similar change here, when component != static_library the > content_* targets are merged into a big content target, and in that case > building content_renderer or any of the smaller targets means building all of > content. So the target is basically defined twice, as a small blob and as a > large blob, and changes have to be made in two places. Done.
mcasas@chromium.org changed reviewers: + jam@chromium.org
jam@ notwithstanding hbos@ LGTM, could you plz RS - content/renderer/BUILD.gn - content/content.gyp (which are basically making the dependency on H264 explicit instead of transitively via WebRtc)
lgtm
lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/1884383002/#ps200001 (title: "hbos@ comments: adding explicit dep to openh264 in content.gyp and content/renderer/BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884383002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884383002/200001
Message was sent while issue was closed.
Description was changed from ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This CL adds a new class H264Encoder <is a> Encoder, where on ToT, there is only VpxEncoder <is a> Encoder. The new encoder is behing #if BUILDFLAG(RTC_USE_H264). Some code shifting from VpxEncoder to Encoder takes place, in particular, some common functionality is extracted from VpxEncoder into Encoder: - StartFrameEncode() and set_paused() are extracted and the latter is made thread safe, then renamed to SetPaused() - Members |main_task_runner_|, |origin_task_runner_| and |encoding_thread_| are also extracted. A few minor renames: - ConfigureEncoding() --> ConfigureEncoderOnEncodingThread() - CalculateFrameDuration() --> EstimateFrameDuration() This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 TEST= see playground CL, also added unittest. ========== to ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This CL adds a new class H264Encoder <is a> Encoder, where on ToT, there is only VpxEncoder <is a> Encoder. The new encoder is behing #if BUILDFLAG(RTC_USE_H264). Some code shifting from VpxEncoder to Encoder takes place, in particular, some common functionality is extracted from VpxEncoder into Encoder: - StartFrameEncode() and set_paused() are extracted and the latter is made thread safe, then renamed to SetPaused() - Members |main_task_runner_|, |origin_task_runner_| and |encoding_thread_| are also extracted. A few minor renames: - ConfigureEncoding() --> ConfigureEncoderOnEncodingThread() - CalculateFrameDuration() --> EstimateFrameDuration() This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 TEST= see playground CL, also added unittest. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This CL adds a new class H264Encoder <is a> Encoder, where on ToT, there is only VpxEncoder <is a> Encoder. The new encoder is behing #if BUILDFLAG(RTC_USE_H264). Some code shifting from VpxEncoder to Encoder takes place, in particular, some common functionality is extracted from VpxEncoder into Encoder: - StartFrameEncode() and set_paused() are extracted and the latter is made thread safe, then renamed to SetPaused() - Members |main_task_runner_|, |origin_task_runner_| and |encoding_thread_| are also extracted. A few minor renames: - ConfigureEncoding() --> ConfigureEncoderOnEncodingThread() - CalculateFrameDuration() --> EstimateFrameDuration() This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 TEST= see playground CL, also added unittest. ========== to ========== VideoTrackRecorder: Adding inner class H264Encoder H264 Encoder using third_party OpenH264 library. This CL adds a new class H264Encoder <is a> Encoder, where on ToT, there is only VpxEncoder <is a> Encoder. The new encoder is behing #if BUILDFLAG(RTC_USE_H264). Some code shifting from VpxEncoder to Encoder takes place, in particular, some common functionality is extracted from VpxEncoder into Encoder: - StartFrameEncode() and set_paused() are extracted and the latter is made thread safe, then renamed to SetPaused() - Members |main_task_runner_|, |origin_task_runner_| and |encoding_thread_| are also extracted. A few minor renames: - ConfigureEncoding() --> ConfigureEncoderOnEncodingThread() - CalculateFrameDuration() --> EstimateFrameDuration() This is the 3rd step in a series of CLs to add support for H264 to MediaRecorder (see https://crrev.com/1886123002/). Next step is to MediaRecorderHandler and Blink support. BUG=601636 TEST= see playground CL, also added unittest. Committed: https://crrev.com/240fa42bc3097277407a3d747bcd26a33e248efa Cr-Commit-Position: refs/heads/master@{#388965} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/240fa42bc3097277407a3d747bcd26a33e248efa Cr-Commit-Position: refs/heads/master@{#388965} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
