Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1437463002:
Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. (Closed)
|
Created:
5 years, 1 month ago by åsapersson Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, andresp, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd tests for vp9 (non-flexible mode) using different spatial and temporal configurations.
Specify kf_min_dist to get correct key frame interval in svc mode.
BUG=chromium:500602
Committed: https://crrev.com/43b48066a7d75bb051eea1e6f451147339cc98a6
Cr-Commit-Position: refs/heads/master@{#10862}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : rebase #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 25
Patch Set 13 : #Patch Set 14 : specify kf_min_dist #
Total comments: 2
Patch Set 15 : #
Total comments: 4
Patch Set 16 : #Patch Set 17 : added comment #Patch Set 18 : rebase #
Messages
Total messages: 38 (22 generated)
Description was changed from ========== Add tests for parsing vp9 using different spatial and temporal configurations. BUG=chromium:500602 ========== to ========== Add tests for parsing vp9 using different spatial and temporal configurations. BUG=chromium:500602 ==========
asapersson@webrtc.org changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
Patchset #6 (id:100001) has been deleted
https://codereview.webrtc.org/1437463002/diff/180001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/180001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1982: TEST_F(VideoSendStreamTest, Vp9NonFlexMode_1Tl1SLayers) { This (and below) looks like something that can be parameterized?
Patchset #10 (id:200001) has been deleted
Patchset #10 (id:220001) has been deleted
Patchset #12 (id:280001) has been deleted
Description was changed from ========== Add tests for parsing vp9 using different spatial and temporal configurations. BUG=chromium:500602 ========== to ========== Add tests for vp9 using different spatial and temporal configurations. BUG=chromium:500602 ==========
Description was changed from ========== Add tests for vp9 using different spatial and temporal configurations. BUG=chromium:500602 ========== to ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations . BUG=chromium:500602 ==========
Description was changed from ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations . BUG=chromium:500602 ========== to ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. BUG=chromium:500602 ==========
Patchset #14 (id:340001) has been deleted
Patchset #13 (id:320001) has been deleted
Patchset #12 (id:300001) has been deleted
https://codereview.webrtc.org/1437463002/diff/180001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/180001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1982: TEST_F(VideoSendStreamTest, Vp9NonFlexMode_1Tl1SLayers) { On 2015/11/10 18:21:19, pbos-webrtc wrote: > This (and below) looks like something that can be parameterized? Changed how this is setup, ptal.
test setup etc. lgtm, stefan@ please help with the bitstream parts :) https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1816: vp9_settings_.frameDroppingOn = false; If you need this put a comment on why. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1817: vp9_settings_.keyFrameInterval = 50; write a kConstant that explains why it's 50 (set it to half of the total transmitted frames, and say that you want to observe a keyframe that resets the structure), maybe put it to 23 or some other prime to make sure that the structure actually has to reset? https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1992: // TODO(asapersson): This is not set correctly for flexible mode. Tracking bug since this is wrong in the current bitstream, so it's not forgotten. :)
Looks like a really useful addition to the tests! https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1854: InspectHeader(parsed.type.Video.codecHeader.VP9); I would perfer a different name for this method. To me VerifyHeader and InspectHeader sounds more or less like the same thing. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1913: EXPECT_NE(kNoTl0PicIdx, vp9.tl0_pic_idx); Should increase by one between each frame, right? Maybe we can check that. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1919: EXPECT_NE(kNoTl0PicIdx, vp9.tl0_pic_idx); Check that it increases by one for each temporal_idx==0 frame. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1925: (!vp9.inter_pic_predicted || last_vp9_.temporal_idx == 1) ? 0 : 1; Maybe a comment to explain this fairly complex expression? Or would it be easier to parse if it was an if/else? I think I in particular want the inter_pic_predicted part of it explained. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1932: EXPECT_GE(vp9.temporal_idx, 0); // 0,2U,1U,2,... Remove the 'U's https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1952: void VerifyTl0idx(const RTPVideoHeaderVP9& vp9) const { VerifTl0Idx https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1966: bool IsFirstPacketInKeyFrame(const RTPVideoHeaderVP9& vp9) const { I'm not sure how the code below verifies that it's a key frame? https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1991: EXPECT_TRUE(vp9.temporal_idx == 0 || vp9.temporal_idx == kNoTemporalIdx); I'm not following this, why can't we have temporal idx > 0 if inter pic prediction is false? May be enough with a comment, or if this method only verifies a certain structure the method name should say so. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2001: EXPECT_LE(vp9.spatial_idx, vp9_settings_.numberOfSpatialLayers - 1); EXPECT_LT(..., vp9_settings_.numberOfSpatialLayers); https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2115: l_field_ = num_temporal_layers_ > 1 || num_spatial_layers_ > 1; I think you can set this in the initializer list too?
Patchset #14 (id:400001) has been deleted
Patchset #13 (id:380001) has been deleted
Patchset #13 (id:420001) has been deleted
Patchset #14 (id:460001) has been deleted
Description was changed from ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. BUG=chromium:500602 ========== to ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. Specify kf_min_dist to get correct key frame interval in svc mode. BUG=chromium:500602 ==========
https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1817: vp9_settings_.keyFrameInterval = 50; On 2015/11/16 16:25:15, pbos-webrtc wrote: > write a kConstant that explains why it's 50 (set it to half of the total > transmitted frames, and say that you want to observe a keyframe that resets the > structure), maybe put it to 23 or some other prime to make sure that the > structure actually has to reset? Changed to constant and added comment. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1854: InspectHeader(parsed.type.Video.codecHeader.VP9); On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > I would perfer a different name for this method. To me VerifyHeader and > InspectHeader sounds more or less like the same thing. Renamed VerifyHeader... https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1913: EXPECT_NE(kNoTl0PicIdx, vp9.tl0_pic_idx); On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > Should increase by one between each frame, right? Maybe we can check that. Checked in VerifyTl0idx (if set). https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1919: EXPECT_NE(kNoTl0PicIdx, vp9.tl0_pic_idx); On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > Check that it increases by one for each temporal_idx==0 frame. see above https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1925: (!vp9.inter_pic_predicted || last_vp9_.temporal_idx == 1) ? 0 : 1; On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > Maybe a comment to explain this fairly complex expression? Or would it be easier > to parse if it was an if/else? I think I in particular want the > inter_pic_predicted part of it explained. added IsKeyFrame https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1932: EXPECT_GE(vp9.temporal_idx, 0); // 0,2U,1U,2,... On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > Remove the 'U's Done. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1952: void VerifyTl0idx(const RTPVideoHeaderVP9& vp9) const { On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > VerifTl0Idx Done. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1966: bool IsFirstPacketInKeyFrame(const RTPVideoHeaderVP9& vp9) const { On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > I'm not sure how the code below verifies that it's a key frame? change to use IsKeyFrame https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1991: EXPECT_TRUE(vp9.temporal_idx == 0 || vp9.temporal_idx == kNoTemporalIdx); On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > I'm not following this, why can't we have temporal idx > 0 if inter pic > prediction is false? May be enough with a comment, or if this method only > verifies a certain structure the method name should say so. If P is zero, the temporal id should be 0, right? https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1992: // TODO(asapersson): This is not set correctly for flexible mode. On 2015/11/16 16:25:15, pbos-webrtc wrote: > Tracking bug since this is wrong in the current bitstream, so it's not > forgotten. :) This has been fixed. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2001: EXPECT_LE(vp9.spatial_idx, vp9_settings_.numberOfSpatialLayers - 1); On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > EXPECT_LT(..., vp9_settings_.numberOfSpatialLayers); Done. https://codereview.webrtc.org/1437463002/diff/360001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2115: l_field_ = num_temporal_layers_ > 1 || num_spatial_layers_ > 1; On 2015/11/16 17:36:30, stefan-webrtc (holmer) wrote: > I think you can set this in the initializer list too? Done.
lgtm, thanks! https://codereview.webrtc.org/1437463002/diff/480001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/480001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2118: static const int kKeyFrameInterval = 31; // Set to < kNumFramesToSend. Maybe comment that it's good to have it coprime to the length of the temporal layer structure, and why? Do as you wish here. :)
https://codereview.webrtc.org/1437463002/diff/480001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/480001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:2118: static const int kKeyFrameInterval = 31; // Set to < kNumFramesToSend. On 2015/11/19 11:53:54, pbos-webrtc wrote: > Maybe comment that it's good to have it coprime to the length of the temporal > layer structure, and why? > > Do as you wish here. :) Done.
https://codereview.webrtc.org/1437463002/diff/500001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/500001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1967: bool IsKeyFrame(const RTPVideoHeaderVP9& vp9) const { I'm not sure if !inter_pic_predicted is enough to say that this is a key frame. It could in theory be an intra frame which doesn't update all reference buffers, or a layer frame which only uses inter-layer prediction, where the layer below is using inter-picture prediction. I saw that we had a TODO in vp9_impl.cc: // TODO(asapersson): Set correct values. vp9_info->inter_pic_predicted = (pkt.data.frame.flags & VPX_FRAME_IS_KEY) ? false : true; Is that TODO referring to the same thing as I mention above?
Patchset #16 (id:520001) has been deleted
https://codereview.webrtc.org/1437463002/diff/500001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1437463002/diff/500001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1967: bool IsKeyFrame(const RTPVideoHeaderVP9& vp9) const { On 2015/11/23 10:27:13, stefan-webrtc (holmer) wrote: > I'm not sure if !inter_pic_predicted is enough to say that this is a key frame. > It could in theory be an intra frame which doesn't update all reference buffers, > or a layer frame which only uses inter-layer prediction, where the layer below > is using inter-picture prediction. > > I saw that we had a TODO in vp9_impl.cc: > // TODO(asapersson): Set correct values. > vp9_info->inter_pic_predicted = > (pkt.data.frame.flags & VPX_FRAME_IS_KEY) ? false : true; > > Is that TODO referring to the same thing as I mention above? For the second example (layer frame using inter-layer pred and layer below using inter-pic pred) I am not sure it would be flagged as a key frame? (And the flag is used to set inter_pic_prediction). I removed this todo. That referred to the todos below: that some values should be obtained from the encoder (temporal up switch, etc). Also removed this function.
File a bug about fixing the TODOs in vp9_impl.cc so that the packetization better matches the bitstream. lgtm https://codereview.webrtc.org/1437463002/diff/500001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1437463002/diff/500001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:562: (pkt.data.frame.flags & VPX_FRAME_IS_KEY) ? false : true; Update comment as discussed.
https://codereview.webrtc.org/1437463002/diff/500001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1437463002/diff/500001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:562: (pkt.data.frame.flags & VPX_FRAME_IS_KEY) ? false : true; On 2015/12/01 14:15:49, stefan-webrtc (holmer) wrote: > Update comment as discussed. Done.
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1437463002/#ps580001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1437463002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437463002/580001
Message was sent while issue was closed.
Description was changed from ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. Specify kf_min_dist to get correct key frame interval in svc mode. BUG=chromium:500602 ========== to ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. Specify kf_min_dist to get correct key frame interval in svc mode. BUG=chromium:500602 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. Specify kf_min_dist to get correct key frame interval in svc mode. BUG=chromium:500602 ========== to ========== Add tests for vp9 (non-flexible mode) using different spatial and temporal configurations. Specify kf_min_dist to get correct key frame interval in svc mode. BUG=chromium:500602 Committed: https://crrev.com/43b48066a7d75bb051eea1e6f451147339cc98a6 Cr-Commit-Position: refs/heads/master@{#10862} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/43b48066a7d75bb051eea1e6f451147339cc98a6 Cr-Commit-Position: refs/heads/master@{#10862}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:580001) has been created in https://codereview.webrtc.org/1492783002/ by asapersson@webrtc.org. The reason for reverting is: Breaks bots. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
