Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1318283007:
H264 Decoder: move cut off logic from Construct to ModifyRefPicList(). (Closed)
|
Created:
5 years, 3 months ago by hshi1 Modified:
5 years, 3 months ago Reviewers:
Pawel Osciak CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionH264 Decoder: move cut off logic from Construct to ModifyRefPicList().
The ConstructRefPicListsP/B() functions are supposed to be slice agnostic.
The cut off logic is based on per-slice num_ref_idx_lX_active_minus1 values
in the slice header, and therefore it should belong to the ModifyRefPicList()
function.
BUG=479953
TEST=verify no corruption on xfnity, trybot, vda unit test
Committed: https://crrev.com/5e19b061b92dd718401660d1899fd56bcd8637f9
Cr-Commit-Position: refs/heads/master@{#346947}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address pawel's comments. #
Total comments: 4
Patch Set 3 : Address more pawel's comments. #Patch Set 4 : Fix typo about the ! #
Total comments: 2
Patch Set 5 : one more nit #Messages
Total messages: 19 (3 generated)
hshi@chromium.org changed reviewers: + posciak@chromium.org
PTAL, verified on Link to fix the corruption.
On 2015/09/01 00:19:32, hshi1 wrote: > PTAL, verified on Link to fix the corruption. Also verified on veyron_minnie with Xfinity and vda unittests
https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... File content/common/gpu/media/h264_decoder.cc (right): https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... content/common/gpu/media/h264_decoder.cc:494: // Cut off if we have more than requested in slice header. It would be nice to keep the comment from l.440 and change this slightly. It's crucial to say here that it is possible and correct to actually not cut off, but enlarge the list with this. The new entries would be empty, i.e. nullptr. If it wasn't allowed, we would have to check that we are not actually enlarging here. And of course nullptr entries on this list are handled in the clients of this class. https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... content/common/gpu/media/h264_decoder.cc:495: ref_pic_listx->resize(slice_hdr->num_ref_idx_l0_active_minus1 + 1); Per spec we should be trimming the list regardless of ref_pic_list_modification_flag_lX, so this should happen before l.489. Similarly for the other list below.
https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... File content/common/gpu/media/h264_decoder.cc (right): https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... content/common/gpu/media/h264_decoder.cc:494: // Cut off if we have more than requested in slice header. On 2015/09/01 11:00:36, Pawel Osciak wrote: > It would be nice to keep the comment from l.440 and change this slightly. > > It's crucial to say here that it is possible and correct to actually not cut > off, but enlarge the list with this. The new entries would be empty, i.e. > nullptr. If it wasn't allowed, we would have to check that we are not actually > enlarging here. And of course nullptr entries on this list are handled in the > clients of this class. I see. Would the verbatim comment from l.440 (slightly reworded here) suffice, or would you prefer it to be more verbose and elaborate on the point of possibility of enlarging the list (and the fact that nullptr entries should not result in a CHECK failure)?
PTAL patch set #2 - addressed your comments and verified that vda unit tests are still passing on link and veyron, and the CL still fixes the corruption. https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... File content/common/gpu/media/h264_decoder.cc (right): https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... content/common/gpu/media/h264_decoder.cc:494: // Cut off if we have more than requested in slice header. On 2015/09/01 11:00:36, Pawel Osciak wrote: > It would be nice to keep the comment from l.440 and change this slightly. > > It's crucial to say here that it is possible and correct to actually not cut > off, but enlarge the list with this. The new entries would be empty, i.e. > nullptr. If it wasn't allowed, we would have to check that we are not actually > enlarging here. And of course nullptr entries on this list are handled in the > clients of this class. Done. https://codereview.chromium.org/1318283007/diff/1/content/common/gpu/media/h2... content/common/gpu/media/h264_decoder.cc:495: ref_pic_listx->resize(slice_hdr->num_ref_idx_l0_active_minus1 + 1); On 2015/09/01 11:00:36, Pawel Osciak wrote: > Per spec we should be trimming the list regardless of > ref_pic_list_modification_flag_lX, so this should happen before l.489. > > Similarly for the other list below. Done.
lgtm % nits https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... File content/common/gpu/media/h264_decoder.cc (right): https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... content/common/gpu/media/h264_decoder.cc:486: // Cut off if we have more than requested in slice header. Hm, this should probably say resize instead of cut off... How about: Resize the list to the size requested in the slice header. Note that per 8.2.4.2 it's possible for num_ref_idx_lX_active_minus1 to indicate there should be more ref pics on list than we constructed. Those superfluous ones should be treated as non-reference and will be initialized to nullptr, which must be handled by clients. https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... content/common/gpu/media/h264_decoder.cc:494: I'm on the fence here, so completely up to you, but what would you think about: bool ref_pic_list_modification_flag_lX; size_t num_ref_idx_lX_active_minus1; if (list == 0) { ref_pic_list_modification_flag_lX = slice_hdr->ref_pic_list_modification_flag_l0; num_ref_idx_lX_active_minus1 = slice_hdr->num_ref_idx_l0_active_minus1; list_mod = slice_hdr->ref_list_l0_modifications; } else { ref_pic_list_modification_flag_lX = slice_hdr->ref_pic_list_modification_flag_l1; num_ref_idx_lX_active_minus1 = slice_hdr->num_ref_idx_l1_active_minus1; list_mod = slice_hdr->ref_list_l1_modifications; } // Comment... ref_pic_listx->resize(num_ref_idx_lX_active_minus1 + 1); if (!ref_pic_list_modification_flag_lX) return true;
PTAL patch set #3 thanks! https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... File content/common/gpu/media/h264_decoder.cc (right): https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... content/common/gpu/media/h264_decoder.cc:486: // Cut off if we have more than requested in slice header. On 2015/09/02 03:34:58, Pawel Osciak wrote: > Hm, this should probably say resize instead of cut off... > > How about: > > Resize the list to the size requested in the slice header. > Note that per 8.2.4.2 it's possible for num_ref_idx_lX_active_minus1 to > indicate there should be more ref pics on list than we constructed. > Those superfluous ones should be treated as non-reference and will be > initialized to nullptr, which must be handled by clients. Done. https://chromiumcodereview.appspot.com/1318283007/diff/20001/content/common/g... content/common/gpu/media/h264_decoder.cc:494: On 2015/09/02 03:34:58, Pawel Osciak wrote: > I'm on the fence here, so completely up to you, but what would you think about: > > bool ref_pic_list_modification_flag_lX; > size_t num_ref_idx_lX_active_minus1; > > if (list == 0) { > ref_pic_list_modification_flag_lX = > slice_hdr->ref_pic_list_modification_flag_l0; > num_ref_idx_lX_active_minus1 = > slice_hdr->num_ref_idx_l0_active_minus1; > list_mod = slice_hdr->ref_list_l0_modifications; > } else { > ref_pic_list_modification_flag_lX = > slice_hdr->ref_pic_list_modification_flag_l1; > num_ref_idx_lX_active_minus1 = > slice_hdr->num_ref_idx_l1_active_minus1; > list_mod = slice_hdr->ref_list_l1_modifications; > } > > // Comment... > ref_pic_listx->resize(num_ref_idx_lX_active_minus1 + 1); > > if (!ref_pic_list_modification_flag_lX) > return true; Done.
Oops sorry I meant patch set #4 (#3 has a typo about the !)
lgtm % nit https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... File content/common/gpu/media/h264_decoder.cc (left): https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... content/common/gpu/media/h264_decoder.cc:510: DCHECK_GE(num_ref_idx_lX_active_minus1, 0); Please keep the DCHECK just in case.
https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... File content/common/gpu/media/h264_decoder.cc (left): https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... content/common/gpu/media/h264_decoder.cc:510: DCHECK_GE(num_ref_idx_lX_active_minus1, 0); On 2015/09/02 03:57:18, Pawel Osciak wrote: > Please keep the DCHECK just in case. Are you sure this is needed? Note that after this CL, num_ref_idx_lX_active_minus1 value is from the slice header, where the value type is unsigned char, so it can never be negative.
On 2015/09/02 03:58:44, hshi1 wrote: > https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... > File content/common/gpu/media/h264_decoder.cc (left): > > https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... > content/common/gpu/media/h264_decoder.cc:510: > DCHECK_GE(num_ref_idx_lX_active_minus1, 0); > On 2015/09/02 03:57:18, Pawel Osciak wrote: > > Please keep the DCHECK just in case. > > Are you sure this is needed? Note that after this CL, > num_ref_idx_lX_active_minus1 value is from the slice header, where the value > type is unsigned char, so it can never be negative. Yes, that's why it's a DCHECK, to guard against programming errors (if that ever changed). If it was possible in runtime, this would have to be a real if().
On 2015/09/02 04:00:29, Pawel Osciak wrote: > On 2015/09/02 03:58:44, hshi1 wrote: > > > https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... > > File content/common/gpu/media/h264_decoder.cc (left): > > > > > https://chromiumcodereview.appspot.com/1318283007/diff/60001/content/common/g... > > content/common/gpu/media/h264_decoder.cc:510: > > DCHECK_GE(num_ref_idx_lX_active_minus1, 0); > > On 2015/09/02 03:57:18, Pawel Osciak wrote: > > > Please keep the DCHECK just in case. > > > > Are you sure this is needed? Note that after this CL, > > num_ref_idx_lX_active_minus1 value is from the slice header, where the value > > type is unsigned char, so it can never be negative. > > Yes, that's why it's a DCHECK, to guard against programming errors (if that ever > changed). > If it was possible in runtime, this would have to be a real if(). Done.
Ready for CQ (tested again with xfinity and unit test)
The CQ bit was checked by hshi@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/1318283007/#ps80001 (title: "one more nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318283007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318283007/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5e19b061b92dd718401660d1899fd56bcd8637f9 Cr-Commit-Position: refs/heads/master@{#346947} |
