Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750633003:
Save width/height of SPS nalus and restore them on the first packet of an IDR. (Closed)
|
Created:
3 years, 9 months ago by philipel Modified:
3 years, 9 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSave width/height of SPS nalus and restore them on the first packet of an IDR.
It appears that for some H264 streams that the width/height is not set for
the first packet of the IDR but in the packet containing the SPS/PPS.
BUG=chromium:698088, webrtc:7139
Review-Url: https://codereview.webrtc.org/2750633003
Cr-Commit-Position: refs/heads/master@{#17239}
Committed: https://chromium.googlesource.com/external/webrtc/+/620d75f5befe1387bc457bc3ec5ad0158c4e6697
Patch Set 1 #Patch Set 2 : More testing. #
Total comments: 10
Patch Set 3 : Feedback #Patch Set 4 : Save width/height for out of band SPS/PPS. #Patch Set 5 : Style fix. #Patch Set 6 : Test fix. #
Messages
Total messages: 23 (12 generated)
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
philipel@webrtc.org changed reviewers: + stefan@webrtc.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:184: if (sps_data_[sps_id].width > 0 && sps_data_[sps_id].height) { height > 0 Or, if you anyway initialize width/height to -1 you might as well always do this assignment? https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:333: // Insert an SPS/PPS packet with widht/height and make sure width https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:342: EXPECT_EQ(H264SpsPpsTracker::kDrop, Can you clarify why this is dropped? https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:359: // widht/height is not overwritten by the tracker. width
https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:184: if (sps_data_[sps_id].width > 0 && sps_data_[sps_id].height) { On 2017/03/14 10:32:20, stefan-webrtc wrote: > height > 0 > > Or, if you anyway initialize width/height to -1 you might as well always do this > assignment? The problem is that I can't be certain that the SPS/PPS packet will have the width/height set (the width, height is always in the SPS, but I save info from the RTP header). My experience with H264 so far is that no two implementations works the same, and the way to get it to work is to make our implementation as forgiving as possible, and this is the more general solution. https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:333: // Insert an SPS/PPS packet with widht/height and make sure On 2017/03/14 10:32:20, stefan-webrtc wrote: > width Done. https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:342: EXPECT_EQ(H264SpsPpsTracker::kDrop, On 2017/03/14 10:32:20, stefan-webrtc wrote: > Can you clarify why this is dropped? Since this packet only contains SPS/PPS we don't want to insert it into the packet buffer. https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:359: // widht/height is not overwritten by the tracker. On 2017/03/14 10:32:20, stefan-webrtc wrote: > width Done.
https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:184: if (sps_data_[sps_id].width > 0 && sps_data_[sps_id].height) { On 2017/03/14 11:51:12, philipel wrote: > On 2017/03/14 10:32:20, stefan-webrtc wrote: > > height > 0 > > > > Or, if you anyway initialize width/height to -1 you might as well always do > this > > assignment? > > The problem is that I can't be certain that the SPS/PPS packet will have the > width/height set (the width, height is always in the SPS, but I save info from > the RTP header). > > My experience with H264 so far is that no two implementations works the same, > and the way to get it to work is to make our implementation as forgiving as > possible, and this is the more general solution. I'm fairly sure width/height is required in the sps nalu, but if you want to be on the safe side... I think my main point was that sps_data_[sps_id].width is initialized to -1, and if there was no width/height in the sps then I'd expect that we also want packet->width to be -1? That would still be what happens if we remove the if statement.
https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2750633003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:184: if (sps_data_[sps_id].width > 0 && sps_data_[sps_id].height) { On 2017/03/14 12:06:36, stefan-webrtc wrote: > On 2017/03/14 11:51:12, philipel wrote: > > On 2017/03/14 10:32:20, stefan-webrtc wrote: > > > height > 0 > > > > > > Or, if you anyway initialize width/height to -1 you might as well always do > > this > > > assignment? > > > > The problem is that I can't be certain that the SPS/PPS packet will have the > > width/height set (the width, height is always in the SPS, but I save info from > > the RTP header). > > > > My experience with H264 so far is that no two implementations works the same, > > and the way to get it to work is to make our implementation as forgiving as > > possible, and this is the more general solution. > > I'm fairly sure width/height is required in the sps nalu, but if you want to be > on the safe side... > > I think my main point was that sps_data_[sps_id].width is initialized to -1, and > if there was no width/height in the sps then I'd expect that we also want > packet->width to be -1? That would still be what happens if we remove the if > statement. I looked at the code and we only know the width/height if we receive an SPS, so yeah, this if check is redundant. But it also made me realize that we need to save this info in InsertSpsPpsNalus.
lgtm
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23088)
Description was changed from ========== Save width/height of SPS nalus and restore them on the first packet of an IDR. It appears that for some H264 streams that the width/height is not set for the first packet of the IDR but in the packet containing the SPS/PPS. BUG=chromium:698088 ========== to ========== Save width/height of SPS nalus and restore them on the first packet of an IDR. It appears that for some H264 streams that the width/height is not set for the first packet of the IDR but in the packet containing the SPS/PPS. BUG=chromium:698088, webrtc:7139 ==========
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2750633003/#ps100001 (title: "Test fix.")
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": 100001, "attempt_start_ts": 1489569820832350,
"parent_rev": "9f8a566316c226e799def9f10eb689f9aeb0a049", "commit_rev":
"620d75f5befe1387bc457bc3ec5ad0158c4e6697"}
Message was sent while issue was closed.
Description was changed from ========== Save width/height of SPS nalus and restore them on the first packet of an IDR. It appears that for some H264 streams that the width/height is not set for the first packet of the IDR but in the packet containing the SPS/PPS. BUG=chromium:698088, webrtc:7139 ========== to ========== Save width/height of SPS nalus and restore them on the first packet of an IDR. It appears that for some H264 streams that the width/height is not set for the first packet of the IDR but in the packet containing the SPS/PPS. BUG=chromium:698088, webrtc:7139 Review-Url: https://codereview.webrtc.org/2750633003 Cr-Commit-Position: refs/heads/master@{#17239} Committed: https://chromium.googlesource.com/external/webrtc/+/620d75f5befe1387bc457bc3e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/620d75f5befe1387bc457bc3e...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.webrtc.org/2754543005/ by philipel@webrtc.org. The reason for reverting is: Breaks build bots.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
