Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814063002:
Update constraints algorithm for screen capture. (Closed)
|
Created:
3 years, 8 months ago by Guido Urdaneta Modified:
3 years, 8 months ago Reviewers:
Max Morin CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, avayvod+watch_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate constraints algorithm for screen capture.
If explicit frame rate is given, use that as default. This is more
compatible with existing tests and applications and is still spec
compliant. This fixes a regression.
Use media::limits::kMaxDimension/2 (16383) as maximum screencast
dimension. The older value could produce results that exceed the
maximum canvas size, which is 16384 x 16384 - 1.
Properly use explicit maximum dimensions and frame rate as default in
screen capture. An older fix for dimensions had a bug when the maximum
allowed values coincided with explicitly given max constraints.
BUG=657733, 710187
Review-Url: https://codereview.chromium.org/2814063002
Cr-Commit-Position: refs/heads/master@{#464068}
Committed: https://chromium.googlesource.com/chromium/src/+/02fcf46036643c6c896c059161f2a2b6aa8e6c6b
Patch Set 1 #Patch Set 2 : reduce max dimension #
Total comments: 2
Patch Set 3 : maxmorin's comments and minor rearrangement of constants #
Messages
Total messages: 31 (21 generated)
The CQ bit was checked by guidou@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by guidou@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...
Description was changed from ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Properly use explicit maximum dimensions and frame rate as default in screen capture. The older fix had a bug when the maximum allowed values were given as explicit max in constraints. BUG=657733,710187 ========== to ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ==========
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_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by guidou@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...
Description was changed from ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ========== to ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Use media::limits::kMaxDimension/2 (16383) as maximum screencast dimension. The older value could produce results that exceed the maximum canvas size, which is 16384 x 16384 - 1. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ==========
guidou@chromium.org changed reviewers: + hbos@chromium.org
Hi, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
guidou@chromium.org changed reviewers: + maxmorin@chromium.org - hbos@chromium.org
Max: can you take a look?
lgtm https://codereview.chromium.org/2814063002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2814063002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:28: // than media::limits::kMaxCanvas. Should there be a static_assert that it's less then kMaxCanvas?
Thanks for the review! https://codereview.chromium.org/2814063002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_content.cc (right): https://codereview.chromium.org/2814063002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_content.cc:28: // than media::limits::kMaxCanvas. On 2017/04/12 15:29:37, Max Morin wrote: > Should there be a static_assert that it's less then kMaxCanvas? Done.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxmorin@chromium.org Link to the patchset: https://codereview.chromium.org/2814063002/#ps40001 (title: "maxmorin's comments and minor rearrangement of constants")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by guidou@chromium.org
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": 40001, "attempt_start_ts": 1492015942606310,
"parent_rev": "e8b8728db9d39906383b4699ed9e6b397b992cea", "commit_rev":
"02fcf46036643c6c896c059161f2a2b6aa8e6c6b"}
Description was changed from ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Use media::limits::kMaxDimension/2 (16383) as maximum screencast dimension. The older value could produce results that exceed the maximum canvas size, which is 16384 x 16384 - 1. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ========== to ========== Update constraints algorithm for screen capture. If explicit max frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Use media::limits::kMaxDimension/2 (16383) as maximum screencast dimension. The older value could produce results that exceed the maximum canvas size, which is 16384 x 16384 - 1. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ==========
Message was sent while issue was closed.
Description was changed from ========== Update constraints algorithm for screen capture. If explicit max frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Use media::limits::kMaxDimension/2 (16383) as maximum screencast dimension. The older value could produce results that exceed the maximum canvas size, which is 16384 x 16384 - 1. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 ========== to ========== Update constraints algorithm for screen capture. If explicit frame rate is given, use that as default. This is more compatible with existing tests and applications and is still spec compliant. This fixes a regression. Use media::limits::kMaxDimension/2 (16383) as maximum screencast dimension. The older value could produce results that exceed the maximum canvas size, which is 16384 x 16384 - 1. Properly use explicit maximum dimensions and frame rate as default in screen capture. An older fix for dimensions had a bug when the maximum allowed values coincided with explicitly given max constraints. BUG=657733,710187 Review-Url: https://codereview.chromium.org/2814063002 Cr-Commit-Position: refs/heads/master@{#464068} Committed: https://chromium.googlesource.com/chromium/src/+/02fcf46036643c6c896c059161f2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/02fcf46036643c6c896c059161f2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
