|
|
DescriptionDetect H264 slices of the same frame in VEAUnittest
This CL adds checks similar to H264Decoder::IsNewPrimaryCodedPicture() to
determine if the given H264 slice belongs to a new frame. On Mac and Win,
we can have multiple slices per frame and this causes problems.
Note that after this fix, I will add this test to chromium.gpu.fyi Mac bots.
Windows will follow when bots get upgraded to Win 8.1.
BUG=669678
TEST=Tested VEAUnittest on Macbook Air 2013.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2538883002
Cr-Commit-Position: refs/heads/master@{#451086}
Committed: https://chromium.googlesource.com/chromium/src/+/e80ab729b5fbe7b6ee7cf44551b0412a2e03f51f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refactor into static methods. #
Total comments: 18
Patch Set 3 : posciak@ comments. #Patch Set 4 : Win clang build issues. #
Total comments: 12
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 57 (42 generated)
Description was changed from ========== fix h264 header. BUG= ========== to ========== fix h264 header. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by emircan@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 ========== fix h264 header. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Detect H264 slices of the same frame in VEAUnittest This CL adds checks similar to H264Decoder::IsNewPrimaryCodedPicture() to determine if the given H264 slice belongs to a new frame. On Mac and Win, we can have multiple slices per frame and this causes problems. Note that after this fix, I will add this test to chromium.gpu.fyi Mac bots. Windows will follow when bots get upgraded to Win 8.1. BUG=669678 TEST=Tested VEAUnittest on Macbook Air 2013. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
emircan@chromium.org changed reviewers: + posciak@google.com
PTAL.
emircan@chromium.org changed reviewers: + posciak@chromium.org - posciak@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2538883002/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:694: bool H264Validator::IsNewPrimaryCodedPicture(const H264SliceHeader* slice_hdr, Perhaps we would be able to turn H264Decoder::IsNewPrimaryCodedPicture() into a static call by passing the previous slice header instead of using curr_pic_ in there as well, and use it here directly?
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2538883002/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:694: bool H264Validator::IsNewPrimaryCodedPicture(const H264SliceHeader* slice_hdr, On 2016/11/30 02:05:22, Pawel Osciak wrote: > Perhaps we would be able to turn H264Decoder::IsNewPrimaryCodedPicture() into a > static call by passing the previous slice header instead of using curr_pic_ in > there as well, and use it here directly? Done.
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Friendly ping
emircan@chromium.org changed reviewers: + wuchengli@chromium.org
Friendly ping again :) Also, adding wuchengli@ to take a look if he has time.
https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.cc File media/gpu/h264_decoder.cc (right): https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1356: bool H264Decoder::InitPictureFromSliceHeader(int curr_sps_id, Perhaps we could instead return a scoped_refptr<H264Picture> to the new picture and return nullptr on failure please? We wouldn't need to pass the existing picture either, and also s/InitPicture/CreateH264Picture/ then. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1357: const H264Parser& parser, Perhaps we could pass the SPS directly to the method to avoid having to pass the parser? In general the method requires an SPS, but not parser functionality. We wouldn't need the sps id either then. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1411: bool H264Decoder::IsNewPrimaryCodedPicture(scoped_refptr<H264Picture> curr_pic, const scoped_refptr<>& ? https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1414: const H264Parser& parser, Here we should be able to also pass an SPS only, without id and parser. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1415: const H264SliceHeader* slice_hdr) { Since we are not changing the structure, could we instead const H264SliceHeader& please? https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:605: curr_pic_(new H264Picture()), Perhaps we could create this when a new frame is parsed. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:606: curr_sps_id_(-1), Nit: perhaps initialize at definition site below? https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:651: if (IsNewFrame(nalu, sps_id, pps_id) && !frame_cb_.Run(keyframe)) For readability, please: if (IsNewFrame()) { if (!frame_cb_.Run(keyframe)) return; } Separately, even if we just saw an SPS and/or PPS in the stream, it does not mean they are being used by the slice that follows them (or, really, by any of the subsequent slices in the stream for that matter). So for the new frame logic, we should instead compare pic_parameter_set_id from the parsed H264SliceHeader and the corresponding seq_parameter_set_id (might be useful to add it to H264SliceHeader as well), to the curr_sps_id_ and curr_pps_id_, and update curr_sps_id_ and curr_pps_id_ with the values from the slice unconditionally, immediately after processing it. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:680: return true; Should this be s/true/false/?
The CQ bit was checked by emircan@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...
https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.cc File media/gpu/h264_decoder.cc (right): https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1356: bool H264Decoder::InitPictureFromSliceHeader(int curr_sps_id, On 2016/12/20 08:34:22, Pawel Osciak wrote: > Perhaps we could instead return a scoped_refptr<H264Picture> to the new picture > and return nullptr on failure please? We wouldn't need to pass the existing > picture either, and also s/InitPicture/CreateH264Picture/ then. Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1357: const H264Parser& parser, On 2016/12/20 08:34:22, Pawel Osciak wrote: > Perhaps we could pass the SPS directly to the method to avoid having to pass the > parser? In general the method requires an SPS, but not parser functionality. We > wouldn't need the sps id either then. Done. Note that I kept param as |const H264SPS* sps| as it looks like there are checks for nullptr within the function. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1411: bool H264Decoder::IsNewPrimaryCodedPicture(scoped_refptr<H264Picture> curr_pic, On 2016/12/20 08:34:22, Pawel Osciak wrote: > const scoped_refptr<>& ? I came across to this in an earlier review that passing scoped_refptr by value is the preferred way now. I couldn't find any code guideline mentioning it thouhg, but there is https://groups.google.com/a/chromium.org/d/msg/chromium-dev/0khIRhxDq6E/1lcud.... https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1414: const H264Parser& parser, On 2016/12/20 08:34:22, Pawel Osciak wrote: > Here we should be able to also pass an SPS only, without id and parser. Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/h264_decoder.... media/gpu/h264_decoder.cc:1415: const H264SliceHeader* slice_hdr) { On 2016/12/20 08:34:22, Pawel Osciak wrote: > Since we are not changing the structure, could we instead const H264SliceHeader& > please? Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:605: curr_pic_(new H264Picture()), On 2016/12/20 08:34:22, Pawel Osciak wrote: > Perhaps we could create this when a new frame is parsed. Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:606: curr_sps_id_(-1), On 2016/12/20 08:34:22, Pawel Osciak wrote: > Nit: perhaps initialize at definition site below? Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:651: if (IsNewFrame(nalu, sps_id, pps_id) && !frame_cb_.Run(keyframe)) On 2016/12/20 08:34:22, Pawel Osciak wrote: > For readability, please: > > if (IsNewFrame()) { > if (!frame_cb_.Run(keyframe)) > return; > } > > > Separately, even if we just saw an SPS and/or PPS in the stream, it does not > mean they are being used by the slice that follows them (or, really, by any of > the subsequent slices in the stream for that matter). > > So for the new frame logic, we should instead compare pic_parameter_set_id from > the parsed H264SliceHeader and the corresponding seq_parameter_set_id (might be > useful to add it to H264SliceHeader as well), to the curr_sps_id_ and > curr_pps_id_, and update curr_sps_id_ and curr_pps_id_ with the values from the > slice unconditionally, immediately after processing it. Done. https://codereview.chromium.org/2538883002/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:680: return true; On 2016/12/20 08:34:22, Pawel Osciak wrote: > Should this be s/true/false/? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by emircan@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: This issue passed the CQ dry run.
The CQ bit was checked by emircan@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...
Patchset #4 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping. I need this fix to land before adding it to chromium.gpu.fyi Mac bots. Currently, we only have JS peerconnection tests that run in chromium.webrtc bots that uses VEa.
https://codereview.chromium.org/2538883002/diff/100001/media/gpu/h264_decoder.h File media/gpu/h264_decoder.h (right): https://codereview.chromium.org/2538883002/diff/100001/media/gpu/h264_decoder... media/gpu/h264_decoder.h:120: // Createsa H264Picture from given params. Return nullptr when there is an s/Createsa/Creates a/ https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:650: const bool par_res = h264_parser_.ParseSliceHeader(nalu, &slice_hdr); s/bool/auto/ https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:651: if (par_res != H264Parser::kOk) { ASSERT_TRUE perhaps? https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:659: if (!UpdateCurrentPicture(slice_hdr)) { This should also likely be a failure. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:701: curr_pic_ = H264Decoder::CreateH264PictureFromSliceHeader( Should we only be Creating if IsNewPicture() returned true, not otherwise? https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:702: h264_parser_.GetSPS(curr_sps_id_), slice_hdr); For symmetry, perhaps we could check GetSPS()'s result similarly to GetPPS() above and fail on that?
The CQ bit was checked by emircan@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...
https://codereview.chromium.org/2538883002/diff/100001/media/gpu/h264_decoder.h File media/gpu/h264_decoder.h (right): https://codereview.chromium.org/2538883002/diff/100001/media/gpu/h264_decoder... media/gpu/h264_decoder.h:120: // Createsa H264Picture from given params. Return nullptr when there is an On 2017/02/14 09:27:34, Pawel Osciak wrote: > s/Createsa/Creates a/ Done. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:650: const bool par_res = h264_parser_.ParseSliceHeader(nalu, &slice_hdr); On 2017/02/14 09:27:34, Pawel Osciak wrote: > s/bool/auto/ Moved it inside ASSERT_EQ. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:651: if (par_res != H264Parser::kOk) { On 2017/02/14 09:27:34, Pawel Osciak wrote: > ASSERT_TRUE perhaps? Moved the whole block into an ASSERT_EQ. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:659: if (!UpdateCurrentPicture(slice_hdr)) { On 2017/02/14 09:27:34, Pawel Osciak wrote: > This should also likely be a failure. Done. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:701: curr_pic_ = H264Decoder::CreateH264PictureFromSliceHeader( On 2017/02/14 09:27:34, Pawel Osciak wrote: > Should we only be Creating if IsNewPicture() returned true, not otherwise? Moved the call inside "if (IsNewPicture(slice_hdr))" block. https://codereview.chromium.org/2538883002/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:702: h264_parser_.GetSPS(curr_sps_id_), slice_hdr); On 2017/02/14 09:27:34, Pawel Osciak wrote: > For symmetry, perhaps we could check GetSPS()'s result similarly to GetPPS() > above and fail on that? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by posciak@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...
lgtm % nit https://codereview.chromium.org/2538883002/diff/120001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/120001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:683: curr_pic_, curr_pps_id_, h264_parser_.GetSPS(curr_sps_id_), slice_hdr); The first time we run this method, curr_pps_id_ is -1, and sps and curr_pic_ will be nullptr, which will result in error messages from the parser. Perhaps we could add a check for !curr_pic_ for example please: if (!curr_pic_) return true; else return H264Decoder::IsNewPrimaryCodedPicture(...);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2538883002/diff/120001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2538883002/diff/120001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:683: curr_pic_, curr_pps_id_, h264_parser_.GetSPS(curr_sps_id_), slice_hdr); On 2017/02/16 09:12:12, Pawel Osciak wrote: > The first time we run this method, curr_pps_id_ is -1, and sps and curr_pic_ > will be nullptr, which will result in error messages from the parser. Perhaps we > could add a check for !curr_pic_ for example please: > > if (!curr_pic_) > return true; > else > return H264Decoder::IsNewPrimaryCodedPicture(...); Thanks for pointing out. Done.
The CQ bit was checked by emircan@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 emircan@chromium.org
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2538883002/#ps140001 (title: " ")
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": 140001, "attempt_start_ts": 1487270290120570, "parent_rev": "0b80485a436c8c881164729adb8484868ecd065d", "commit_rev": "e80ab729b5fbe7b6ee7cf44551b0412a2e03f51f"}
Message was sent while issue was closed.
Description was changed from ========== Detect H264 slices of the same frame in VEAUnittest This CL adds checks similar to H264Decoder::IsNewPrimaryCodedPicture() to determine if the given H264 slice belongs to a new frame. On Mac and Win, we can have multiple slices per frame and this causes problems. Note that after this fix, I will add this test to chromium.gpu.fyi Mac bots. Windows will follow when bots get upgraded to Win 8.1. BUG=669678 TEST=Tested VEAUnittest on Macbook Air 2013. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Detect H264 slices of the same frame in VEAUnittest This CL adds checks similar to H264Decoder::IsNewPrimaryCodedPicture() to determine if the given H264 slice belongs to a new frame. On Mac and Win, we can have multiple slices per frame and this causes problems. Note that after this fix, I will add this test to chromium.gpu.fyi Mac bots. Windows will follow when bots get upgraded to Win 8.1. BUG=669678 TEST=Tested VEAUnittest on Macbook Air 2013. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2538883002 Cr-Commit-Position: refs/heads/master@{#451086} Committed: https://chromium.googlesource.com/chromium/src/+/e80ab729b5fbe7b6ee7cf44551b0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e80ab729b5fbe7b6ee7cf44551b0...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2707793002/ by kcwu@chromium.org. The reason for reverting is: This CL breaks many CTS/GTS tests. crbug.com/694094. |