Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468903003:
Remove webrtc::Video from H264 encoder internals (Closed)
|
Created:
4 years, 1 month ago by hta-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove webrtc::Video from H264 encoder internals
This CL replaces the use of webrtc::Video as an internal
variable in the H.264 encoder with the specific fields
that are used by this encoder.
In support of refactorings discussed around:
BUG=600254
Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c
Committed: https://crrev.com/5881d552c5f88e6b5ffb463d8fec07055f176758
Cr-Original-Commit-Position: refs/heads/master@{#14887}
Cr-Commit-Position: refs/heads/master@{#14892}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Name and unit changes #
Total comments: 1
Patch Set 3 : Another rename (reland) #
Messages
Total messages: 38 (22 generated)
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 ==========
hta@webrtc.org changed reviewers: + magjed@webrtc.org
See if this makes sense to you.
The CQ bit was checked by hta@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hta@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9739)
hta@webrtc.org changed reviewers: + stefan@webrtc.org
Adding stefan as modules/video_coding/codecs/OWNERS
lgtm % nit https://codereview.webrtc.org/2468903003/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right): https://codereview.webrtc.org/2468903003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:82: int keyFrameInterval_; I would prefer to get rid of the camelCase while you're at it.
PTAL - I changed stuff a little more. The static_cast<float> was bothering me. https://codereview.webrtc.org/2468903003/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right): https://codereview.webrtc.org/2468903003/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:82: int keyFrameInterval_; On 2016/11/02 11:14:21, stefan-webrtc (holmer) wrote: > I would prefer to get rid of the camelCase while you're at it. Done. While I was at it I changed the types to match what they are used for within the module rather than what they are passed in at, and made the casts / multiplications where they are set.
The CQ bit was checked by hta@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm again % nit https://codereview.webrtc.org/2468903003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right): https://codereview.webrtc.org/2468903003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:78: unsigned int max_bits_per_second_; Feel free to call target_bps_ and max_bps_. It's a commonly used abbreviation in the code base.
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2468903003/#ps20001 (title: "Name and unit changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/11/02 13:34:32, stefan-webrtc (holmer) wrote: > lgtm again % nit > > https://codereview.webrtc.org/2468903003/diff/20001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right): > > https://codereview.webrtc.org/2468903003/diff/20001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:78: unsigned int > max_bits_per_second_; > Feel free to call target_bps_ and max_bps_. It's a commonly used abbreviation in > the code base. Done. (197 bps and 15 bits_per_second :-)
Message was sent while issue was closed.
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.webrtc.org/2472673002/ by hta@webrtc.org. The reason for reverting is: Landed the wrong patchset. Nothing broken..
Message was sent while issue was closed.
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ==========
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2468903003/#ps40001 (title: "Another rename (reland)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Cr-Commit-Position: refs/heads/master@{#14887} ========== to ========== Remove webrtc::Video from H264 encoder internals This CL replaces the use of webrtc::Video as an internal variable in the H.264 encoder with the specific fields that are used by this encoder. In support of refactorings discussed around: BUG=600254 Committed: https://crrev.com/2549437b5ccf5aae2e6f1a1491c5f505d1859f9c Committed: https://crrev.com/5881d552c5f88e6b5ffb463d8fec07055f176758 Cr-Original-Commit-Position: refs/heads/master@{#14887} Cr-Commit-Position: refs/heads/master@{#14892} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5881d552c5f88e6b5ffb463d8fec07055f176758 Cr-Commit-Position: refs/heads/master@{#14892} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
