Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499003002:
H264 encoder: Include QP information in encoded images (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionH264 encoder: Include QP information in encoded images
Set the |qp_| field in EncodedImage before calling OnEncodedImage.
BUG=webrtc:6541
Committed: https://crrev.com/2d60e53ad5dde01a9d2be29ba20f052f979c3491
Cr-Commit-Position: refs/heads/master@{#15199}
Patch Set 1 #Patch Set 2 : Set QP for HW encoders as well. #
Messages
Total messages: 34 (20 generated)
The CQ bit was checked by magjed@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/...
Patchset #1 (id:1) has been deleted
Description was changed from ========== H264 encoder: Include QP information in encoded images BUG=None ========== to ========== H264 encoder: Include QP information in encoded images BUG=None ==========
magjed@webrtc.org changed reviewers: + kthelgason@webrtc.org, sakal@webrtc.org, stefan@webrtc.org
Description was changed from ========== H264 encoder: Include QP information in encoded images BUG=None ========== to ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=None ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by magjed@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/...
Patchset #1 (id:20001) has been deleted
Please take a look.
On 2016/11/14 14:06:29, magjed_webrtc wrote: > Please take a look. Can you also add this for the VideoToolboxEncoder and MediaCodec in the same CL?
The CQ bit was checked by magjed@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/...
On 2016/11/14 14:21:41, kthelgason wrote: > On 2016/11/14 14:06:29, magjed_webrtc wrote: > > Please take a look. > > Can you also add this for the VideoToolboxEncoder and MediaCodec in the same CL? Thanks, missed that!
lgtm I tested this locally on Android and it correctly populated the qpSum field.
lgtm
Description was changed from ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=None ========== to ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=webrtc:6541 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any chance we can add a test?
On 2016/11/15 08:50:01, stefan-webrtc (holmer) wrote: > Any chance we can add a test? We don't have any existing tests right now that exercise the Android or iOS H264 HW encoders. I can't find any test that exercises h264_encoder_impl.cc either. Do you know of one? Otherwise it feels out of scope to create encoder tests in this CL.
On 2016/11/15 14:58:00, magjed_webrtc wrote: > On 2016/11/15 08:50:01, stefan-webrtc (holmer) wrote: > > Any chance we can add a test? > > We don't have any existing tests right now that exercise the Android or iOS H264 > HW encoders. I can't find any test that exercises h264_encoder_impl.cc either. > Do you know of one? Otherwise it feels out of scope to create encoder tests in > this CL. Stefan - ping. I just want to land this to get the QP stats working for H264.
Tests are probably out of scope, yes... :( But maybe we should plan to add unittests for this wrapper next quarter or so? Would be doable if we had a super thin wrapper around the media codec apis that we could mock. lgtm
Btw, I think we have unittests for vp9 and vp8 wrappers, so it shouldn't be hard to add for h264_encoder_impl.cc too. It's worse with the code relying on HW encoders though.
On 2016/11/22 16:05:55, stefan-webrtc (holmer) wrote: > Btw, I think we have unittests for vp9 and vp8 wrappers, so it shouldn't be hard > to add for h264_encoder_impl.cc too. It's worse with the code relying on HW > encoders though. I'll add it to my TODO-list to look into this.
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479831481760750,
"parent_rev": "570061b7be68eb967198d3d09227f15293558432", "commit_rev":
"01c6da242d39aaa896471d2fc9fda7692397f0a4"}
Message was sent while issue was closed.
Description was changed from ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=webrtc:6541 ========== to ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=webrtc:6541 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=webrtc:6541 ========== to ========== H264 encoder: Include QP information in encoded images Set the |qp_| field in EncodedImage before calling OnEncodedImage. BUG=webrtc:6541 Committed: https://crrev.com/2d60e53ad5dde01a9d2be29ba20f052f979c3491 Cr-Commit-Position: refs/heads/master@{#15199} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2d60e53ad5dde01a9d2be29ba20f052f979c3491 Cr-Commit-Position: refs/heads/master@{#15199} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
