Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2661423002:
VTVDA: Optimize pic_order_cnt_type == 2. (Closed)
|
Created:
3 years, 10 months ago by sandersd (OOO until July 31) Modified:
3 years, 9 months ago Reviewers:
Pawel Osciak CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, mac-reviews_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVTVDA: Optimize pic_order_cnt_type == 2.
This CL also fixes the behavior of VTVDA with regards to MMCO5, which
should be handled like an IDR for reordering purposes. This follows
(perhaps not obviously) from the spec definition of "picture order
count":
'A variable that is associated with each coded field and each field of
a coded frame and has a value that is non-decreasing with increasing
field position in output order relative to the first output field of
the previous IDR picture in decoding order or relative to the first
output field of the previous picture, in decoding order, that contains
a memory management control operation that marks all reference
pictures as "unused for reference".'
This definition conveniently guarantees that when pic_order_cnt_type is
2, then decode order == presentation order (as one would hope).
BUG=webrtc:7010
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2661423002
Cr-Commit-Position: refs/heads/master@{#456889}
Committed: https://chromium.googlesource.com/chromium/src/+/52dceecdb0a9da763a6f62d0c5b010843692d489
Patch Set 1 #Patch Set 2 : Switch from static to namespace {} #Patch Set 3 : Change POC of MMCO5 frame to 0. #
Total comments: 14
Patch Set 4 : Address comments. #Patch Set 5 : Explain MMCO5 handling in the code. #Patch Set 6 : Fix comment typo. #Patch Set 7 : Fix compile. #Patch Set 8 : Update unit test. #
Messages
Total messages: 28 (17 generated)
Description was changed from ========== VTVDA: Optimize pic_order_cnt_type == 2. BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=7010 ========== to ========== VTVDA: Optimize pic_order_cnt_type == 2. BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=7010 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 ==========
sandersd@chromium.org changed reviewers: + posciak@chromium.org
This change treats the frame after a MMCO5 very much like an IDR for reordering purposes.
On 2017/02/01 00:00:57, sandersd wrote: > This change treats the frame after a MMCO5 very much like an IDR for reordering > purposes. posciak@: ping.
Perhaps we could consider adding some more context/explanation to the CL message/code comments please, as this is a complicated topic. Thank you. https://codereview.chromium.org/2661423002/diff/40001/media/gpu/vt_video_deco... File media/gpu/vt_video_decode_accelerator_mac.cc (right): https://codereview.chromium.org/2661423002/diff/40001/media/gpu/vt_video_deco... media/gpu/vt_video_decode_accelerator_mac.cc:953: (!task.frame->is_idr || !task.frame->has_mmco5 || Should this be (!task.frame->is_idr && !task.frame->has_mmco5)? Perhaps a comment here would be helpful. https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc File media/video/h264_poc.cc (left): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:70: // Note: Duplicate frame numbers are ignored. They occur in many videos Should this first note be removed as well? https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc File media/video/h264_poc.cc (right): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:63: int32_t *pic_order_cnt) { On a side note, perhaps we should consider using base::Optional<> for return value? https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:69: bool mmco5 = HasMMCO5(slice_hdr); Could we use pending_mmco5_ instead of mmco5 and remove the latter? https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:114: // Compute POC. Perhaps we could replace this comment with an explanation why this is done (possibly with a reference) please. https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:218: int32_t temp_pic_order_count; Perhaps we could say: if (mmco5) *pic_order_cnt = 0; else { // the whole if-else sequence below } ? https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.h File media/video/h264_poc.h (right): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.h#... media/video/h264_poc.h:29: bool IsPendingMMCO5() { return pending_mmco5_; } Please add documentation.
Description was changed from ========== VTVDA: Optimize pic_order_cnt_type == 2. BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=7010 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 ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=7010 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 ==========
https://codereview.chromium.org/2661423002/diff/40001/media/gpu/vt_video_deco... File media/gpu/vt_video_decode_accelerator_mac.cc (right): https://codereview.chromium.org/2661423002/diff/40001/media/gpu/vt_video_deco... media/gpu/vt_video_decode_accelerator_mac.cc:953: (!task.frame->is_idr || !task.frame->has_mmco5 || On 2017/02/17 04:53:26, Pawel Osciak wrote: > Should this be (!task.frame->is_idr && !task.frame->has_mmco5)? > Perhaps a comment here would be helpful. I expanded the code, hopefully it is clear now. https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc File media/video/h264_poc.cc (left): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:70: // Note: Duplicate frame numbers are ignored. They occur in many videos On 2017/02/17 04:53:26, Pawel Osciak wrote: > Should this first note be removed as well? Yes, it was just plain wrong. (Duplicate frame numbers are allowed by the spec for non-reference frames.) https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc File media/video/h264_poc.cc (right): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:63: int32_t *pic_order_cnt) { On 2017/02/17 04:53:26, Pawel Osciak wrote: > On a side note, perhaps we should consider using base::Optional<> for return > value? Acknowledged. https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:218: int32_t temp_pic_order_count; On 2017/02/17 04:53:26, Pawel Osciak wrote: > Perhaps we could say: > > if (mmco5) > *pic_order_cnt = 0; > else { > // the whole if-else sequence below > } > > ? I wrote it that way originally, but ended up switching back for symmetry with the other POC types. I think it's more clear to break up the steps like this, as they are in the spec, but the difference is trivial. I'll switch it if you ask again ;-). https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.h File media/video/h264_poc.h (right): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.h#... media/video/h264_poc.h:29: bool IsPendingMMCO5() { return pending_mmco5_; } On 2017/02/17 04:53:26, Pawel Osciak wrote: > Please add documentation. Done.
https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc File media/video/h264_poc.cc (right): https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:69: bool mmco5 = HasMMCO5(slice_hdr); On 2017/02/17 04:53:26, Pawel Osciak wrote: > Could we use pending_mmco5_ instead of mmco5 and remove the latter? Yes, it's the same. I did it this way so that member variable mutation only occurs in the sections labelled 'Store state.' https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... media/video/h264_poc.cc:114: // Compute POC. On 2017/02/17 04:53:26, Pawel Osciak wrote: > Perhaps we could replace this comment with an explanation why this is done > (possibly with a reference) please. Done. I went with something similar to what the header file says.
Description was changed from ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=7010 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 ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=webrtc:7010 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 ==========
On 2017/02/22 01:39:32, sandersd (OOO until March 13) wrote: > https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc > File media/video/h264_poc.cc (right): > > https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... > media/video/h264_poc.cc:69: bool mmco5 = HasMMCO5(slice_hdr); > On 2017/02/17 04:53:26, Pawel Osciak wrote: > > Could we use pending_mmco5_ instead of mmco5 and remove the latter? > > Yes, it's the same. I did it this way so that member variable mutation only > occurs in the sections labelled 'Store state.' > > https://codereview.chromium.org/2661423002/diff/40001/media/video/h264_poc.cc... > media/video/h264_poc.cc:114: // Compute POC. > On 2017/02/17 04:53:26, Pawel Osciak wrote: > > Perhaps we could replace this comment with an explanation why this is done > > (possibly with a reference) please. > > Done. I went with something similar to what the header file says. Ping.
Thanks, lgtm.
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: 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_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
emircan@chromium.org changed reviewers: - emircan@chromium.org
Description was changed from ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=webrtc:7010 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 ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=webrtc:7010 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 sandersd@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/2661423002/#ps120001 (title: "Fix compile.")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sandersd@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/2661423002/#ps140001 (title: "Update unit test.")
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": 1489527609019590,
"parent_rev": "01bf47ad61ffef17d3a4e96ada6e2703ae67cf18", "commit_rev":
"52dceecdb0a9da763a6f62d0c5b010843692d489"}
Message was sent while issue was closed.
Description was changed from ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=webrtc:7010 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== VTVDA: Optimize pic_order_cnt_type == 2. This CL also fixes the behavior of VTVDA with regards to MMCO5, which should be handled like an IDR for reordering purposes. This follows (perhaps not obviously) from the spec definition of "picture order count": 'A variable that is associated with each coded field and each field of a coded frame and has a value that is non-decreasing with increasing field position in output order relative to the first output field of the previous IDR picture in decoding order or relative to the first output field of the previous picture, in decoding order, that contains a memory management control operation that marks all reference pictures as "unused for reference".' This definition conveniently guarantees that when pic_order_cnt_type is 2, then decode order == presentation order (as one would hope). BUG=webrtc:7010 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2661423002 Cr-Commit-Position: refs/heads/master@{#456889} Committed: https://chromium.googlesource.com/chromium/src/+/52dceecdb0a9da763a6f62d0c5b0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/52dceecdb0a9da763a6f62d0c5b0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
