Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832273002:
Media Capabilities encoding: wire the hardware encoding support (Closed)
|
Created:
3 years, 8 months ago by mcasas Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, emircan+watch+mediarecorder_chromium.org, posciak+watch_chromium.org, jam, darin-cc_chromium.org, mcasas+mediarecorder_chromium.org, chcunningham Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Capabilities encoding: wire the hardware encoding support
This CL wires the EncodingInfo() response param |smooth| to the
result of a new method in VideoTrackRecorder, namely
IsEncodingLikelyAccelerated() that simply surfaces the previously
somewhat internal information. A couple of extra lines come from
refactoring similar code in either of the classes.
Testing it with https://codepen.io/miguelao/full/bWNwej.
BUG=709181
Review-Url: https://codereview.chromium.org/2832273002
Cr-Commit-Position: refs/heads/master@{#468558}
Committed: https://chromium.googlesource.com/chromium/src/+/8dfce7635bf351e5ead9e6607bd068de121b1f92
Patch Set 1 : #
Total comments: 7
Patch Set 2 : s/IsEncodingLikelyAccelerated/CanUseAcceleratedEncoder/ #Patch Set 3 : chcunningham@s comments: better |smooth| marking and rebase. #
Total comments: 2
Patch Set 4 : Reshuffled |kNumPixelsPerSecondSmoothnessThreshold| and comment #Patch Set 5 : moved base::SysInfo::IsLowEndDevice back inside a method #
Messages
Total messages: 30 (16 generated)
Description was changed from ========== Media Capabilities encoding: wire the hardware encoding support wip wip wip Wiring up VEA support BUG=709181 ========== to ========== Media Capabilities encoding: wire the hardware encoding support This CL wires the EncodingInfo() response param |smooth| to the result of a new method in VideoTrackRecorder, namely IsEncodingLikelyAccelerated() that simply surfaces the previously somewhat internal information. A couple of extra lines come from refactoring similar code in either of the classes. BUG=709181 ==========
Description was changed from ========== Media Capabilities encoding: wire the hardware encoding support This CL wires the EncodingInfo() response param |smooth| to the result of a new method in VideoTrackRecorder, namely IsEncodingLikelyAccelerated() that simply surfaces the previously somewhat internal information. A couple of extra lines come from refactoring similar code in either of the classes. BUG=709181 ========== to ========== Media Capabilities encoding: wire the hardware encoding support This CL wires the EncodingInfo() response param |smooth| to the result of a new method in VideoTrackRecorder, namely IsEncodingLikelyAccelerated() that simply surfaces the previously somewhat internal information. A couple of extra lines come from refactoring similar code in either of the classes. Testing it with https://codepen.io/miguelao/full/bWNwej. BUG=709181 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ PTAL (chcunningham@ -in CC- has a parallel CL that should be orthogonal to these changes).
chcunningham@chromium.org changed reviewers: + chcunningham@chromium.org
Drive by https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/media_recorder_handler.cc:331: info->smooth = VideoTrackRecorder::IsEncodingLikelyAccelerated( Please build this out more to consider system specs and requested resolution etc. We really do not want "smooth" to simply proxy "accelerated". Software codecs are often just as performant. Power efficient (obviously ambiguous) is more likely to proxy "accelerated". Even still, for decoding, we've seen that power advantages of hw acceleration tend to disappear under a certain resolution threshold (the specifics matter, but I think this was h264 vs vp9 at 720p and under). My tentative plan is to return power_efficient = true whenever sw and hw perform roughly the same, otherwise returning true just for hw accelerated.
https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/media_recorder_handler.cc:67: #if BUILDFLAG(RTC_USE_H264) We should use IS_H264_SUPPORTED as the android cl lands. https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/media_recorder_handler.cc:331: info->smooth = VideoTrackRecorder::IsEncodingLikelyAccelerated( On 2017/04/24 20:58:17, chcunningham wrote: > Please build this out more to consider system specs and requested resolution > etc. We really do not want "smooth" to simply proxy "accelerated". Software > codecs are often just as performant. > > Power efficient (obviously ambiguous) is more likely to proxy "accelerated". > Even still, for decoding, we've seen that power advantages of hw acceleration > tend to disappear under a certain resolution threshold (the specifics matter, > but I think this was h264 vs vp9 at 720p and under). My tentative plan is to > return power_efficient = true whenever sw and hw perform roughly the same, > otherwise returning true just for hw accelerated. We do not have power metrics currently, so we can leave it a default value for now, if there is any. Smooth is true for all HW encoders available as we try to initialize a session before reporting support at all. It is not true for some SW encoders as the algorithms do not scale well, i.e 4K SW VP8 encoding don't go beyond ~4 fps[0]. We definitely should definitely make a decision using video_configuration.width/height on SW. I am OK leaving it for another CL. [0] https://goo.gl/Kb4rP8 https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:419: IsEncodingLikelyAccelerated(codec, input_size.width(), This serves as a decision mechanism of accelerated or not, so we should rename to CanUseHardwareEncoder()?
ptal (disregard the red bots, it's due to another CL landing-being reverted all the time). https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/media_recorder_handler.cc:331: info->smooth = VideoTrackRecorder::IsEncodingLikelyAccelerated( On 2017/04/25 02:04:30, emircan wrote: > On 2017/04/24 20:58:17, chcunningham wrote: > > Please build this out more to consider system specs and requested resolution > > etc. Please note that IsEncodingLikelyAccelerated() includes codec _and_ resolutions, and in the implementation we return false if the resolution is not larger than a minimum, which is platform dependent. IOW we skip hw encoder if the resolution is too small, which is essentially what you're saying below. > We really do not want "smooth" to simply proxy "accelerated". Software > > codecs are often just as performant. > > > > Power efficient (obviously ambiguous) is more likely to proxy "accelerated". > > Even still, for decoding, we've seen that power advantages of hw acceleration > > tend to disappear under a certain resolution threshold (the specifics matter, > > but I think this was h264 vs vp9 at 720p and under). My tentative plan is to > > return power_efficient = true whenever sw and hw perform roughly the same, > > otherwise returning true just for hw accelerated. > > We do not have power metrics currently, so we can leave it a default value for > now, if there is any. > Smooth is true for all HW encoders available as we try to initialize a session > before reporting support at all. It is not true for some SW encoders as the > algorithms do not scale well, i.e 4K SW VP8 encoding don't go beyond ~4 fps[0]. > We definitely should definitely make a decision using > video_configuration.width/height on SW. I am OK leaving it for another CL. > > [0] https://goo.gl/Kb4rP8 I guess the real question is the meaning of |smooth| and |power_efficient|. I'm using |smooth| if VEA encoder is available, but chcunningham@ says that's not OK. We coud use |power_efficient| and leave |smooth| to convey other information, e.g. framerate-dependent queries? https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:419: IsEncodingLikelyAccelerated(codec, input_size.width(), On 2017/04/25 02:04:30, emircan wrote: > This serves as a decision mechanism of accelerated or not, so we should rename > to CanUseHardwareEncoder()? CanUseAcceleratedEncoder() ?
https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... content/renderer/media_recorder/media_recorder_handler.cc:331: info->smooth = VideoTrackRecorder::IsEncodingLikelyAccelerated( Totally fine for the initial implementation to be missing some things; follow up here should be P1 and block shipping the feature. As it stands you will be claiming "not smooth" for a significant chunk of recording sessions that may in fact be buttery smooth. We made a deliberate choice not to simply expose "accelerated" as a flag for media capabilities because it focuses on the wrong thing. > IOW we skip hw encoder if the resolution is too small, which is essentially what you're saying below. Thats not exactly what I'm looking for. For instance, in such a case you will mark smooth = false (due to lack of acceleration), but this likely incorrect. Generally SW should easily manage smooth encoding of small frame sizes. It may also do fine with larger frame sizes.
On 2017/04/25 18:52:52, chcunningham wrote: > https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... > File content/renderer/media_recorder/media_recorder_handler.cc (right): > > https://codereview.chromium.org/2832273002/diff/60001/content/renderer/media_... > content/renderer/media_recorder/media_recorder_handler.cc:331: info->smooth = > VideoTrackRecorder::IsEncodingLikelyAccelerated( > Totally fine for the initial implementation to be missing some things; follow up > here should be P1 and block shipping the feature. > > As it stands you will be claiming "not smooth" for a significant chunk of > recording sessions that may in fact be buttery smooth. We made a deliberate > choice not to simply expose "accelerated" as a flag for media capabilities > because it focuses on the wrong thing. OIC what you mean. Then what I propose is to have a better criteria: if resolution < a given one - smooth if framerate < a given one is resolution > a given one - smooth if VEA enabled OR framerate < a given one Where these parameters are possibly platform dependent. > > > IOW we skip hw encoder if the resolution is too small, which is essentially > what you're saying below. > > Thats not exactly what I'm looking for. For instance, in such a case you will > mark smooth = false (due to lack of acceleration), but this likely incorrect. > Generally SW should easily manage smooth encoding of small frame sizes. It may > also do fine with larger frame sizes.
Took chcunningham@s commments in, ptal
On 2017/04/27 22:41:57, mcasas wrote: > Took chcunningham@s commments in, ptal ping reviewers.
https://codereview.chromium.org/2832273002/diff/100001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/100001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler.cc:330: base::SysInfo::IsLowEndDevice() ? 640 * 480 * 30.0 : 1280 * 720 * 30.0; Can we define this as constant at the top of this file? It would be easier with naming.
LGTM. Exciting to see it come to life.
emircan@ PTAL (moved the constant definition and reshuffled the comments). https://codereview.chromium.org/2832273002/diff/100001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2832273002/diff/100001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler.cc:330: base::SysInfo::IsLowEndDevice() ? 640 * 480 * 30.0 : 1280 * 720 * 30.0; On 2017/05/01 17:12:29, emircan wrote: > Can we define this as constant at the top of this file? It would be easier with > naming. Done.
lgtm
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2832273002/#ps160001 (title: "moved base::SysInfo::IsLowEndDevice back inside a method")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493696541862530,
"parent_rev": "0784842ece8911097e2675e08666b44896ee7ab2", "commit_rev":
"8dfce7635bf351e5ead9e6607bd068de121b1f92"}
Message was sent while issue was closed.
Description was changed from ========== Media Capabilities encoding: wire the hardware encoding support This CL wires the EncodingInfo() response param |smooth| to the result of a new method in VideoTrackRecorder, namely IsEncodingLikelyAccelerated() that simply surfaces the previously somewhat internal information. A couple of extra lines come from refactoring similar code in either of the classes. Testing it with https://codepen.io/miguelao/full/bWNwej. BUG=709181 ========== to ========== Media Capabilities encoding: wire the hardware encoding support This CL wires the EncodingInfo() response param |smooth| to the result of a new method in VideoTrackRecorder, namely IsEncodingLikelyAccelerated() that simply surfaces the previously somewhat internal information. A couple of extra lines come from refactoring similar code in either of the classes. Testing it with https://codepen.io/miguelao/full/bWNwej. BUG=709181 Review-Url: https://codereview.chromium.org/2832273002 Cr-Commit-Position: refs/heads/master@{#468558} Committed: https://chromium.googlesource.com/chromium/src/+/8dfce7635bf351e5ead9e6607bd0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/8dfce7635bf351e5ead9e6607bd0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
