Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470133002:
Add function for getting supported H264 level from max resolution and fps (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd function for getting supported H264 level from max resolution and fps
BUG=webrtc:6337
Committed: https://crrev.com/a92704e6f5e240b4656d58a39c10267a8deaaff0
Cr-Commit-Position: refs/heads/master@{#14969}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Return rtc::Optional<Level> instead #
Total comments: 7
Patch Set 3 : Rebase #Patch Set 4 : Improve comments. #
Total comments: 2
Patch Set 5 : Change fps type to float #
Messages
Total messages: 31 (16 generated)
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
Description was changed from ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ========== to ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ==========
magjed@webrtc.org changed reviewers: + hta@webrtc.org, kthelgason@webrtc.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
lgtm with a comment/suggestion. https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... webrtc/common_video/h264/profile_level_id.cc:75: const int max_macroblock_frame_size; super-nit: maybe use size_t for these unless there is a good reason not to. Not worth updating CL just for that though. https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... webrtc/common_video/h264/profile_level_id.cc:175: return kLevel1; Is this really the correct thing to do in this situation? If we reach this point obviously there was something wrong with the input to the function, perhaps it would be better to somehow signal that?
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... webrtc/common_video/h264/profile_level_id.cc:75: const int max_macroblock_frame_size; On 2016/11/02 17:26:21, kthelgason wrote: > super-nit: maybe use size_t for these unless there is a good reason not to. Not > worth updating CL just for that though. This is a static table so we can statically verify it won't overflow. We store pixel counts in ints in a lot of places in WebRTC, but maybe we should change to size_t at some point (maybe 198000x72000 resolution isn't too far away). https://codereview.webrtc.org/2470133002/diff/1/webrtc/common_video/h264/prof... webrtc/common_video/h264/profile_level_id.cc:175: return kLevel1; On 2016/11/02 17:26:21, kthelgason wrote: > Is this really the correct thing to do in this situation? If we reach this point > obviously there was something wrong with the input to the function, perhaps it > would be better to somehow signal that? No, it's not strictly correct. I could do NOT_REACHED here, but it's an ugly requirement that the input should cover at least level 1. I decided to return an rtc::Optional instead and added some tests for it.
Description was changed from ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ========== to ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 TBR=hta@webrtc.org ==========
Harald - I'm TBR:ing you since I'm blocked on this CL. I need to have it in Chromium in order to set correct profile-level-id for their external encoder, and then I can reland the profile-level-id negotiation.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2470133002/#ps40001 (title: "Return rtc::Optional<Level> instead")
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: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12178) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14513) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14396) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19744) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18430) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9795)
Some commentary, so not LGTMing yet. I think you need to specify whether you're converting for encoder or decoder, because the calculations will be different. https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:79: // This is from the H264 spec. Please add the formal reference - ITU.T H.264(year) section <whatever>. These specs are humongous, and have a nasty tendency to change over time. https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:66: // return no level if not even level 1 is supported. This can be made clearer. Something like: Given that a decoder supports up to a given frame size (in pixels) at up to a given number of frames per second, return the highest H.264 level where it can guarantee that it will be able to support all valid encoded streams that are within that level. For an encoder, I think the logic will be different - you would want the level that would guarantee that if the other end agreed to handle it, you could drive the encoder to maximum performance without overrunning the decoder. Thus, if 1280*720@30 is slightly more than Level 3.1 but less than level 4.1: SupportedDecoderLevel(1280*720, 30) = Level3.1 SupportedEncoderLevel(1280*720, 30) = Level4.1 https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id_unittest.cc (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:83: EXPECT_FALSE(SupportedLevel(183 * 137, 30)); Please add a comment as to why these resolution/frame combinations are unable to support any level.
Description was changed from ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 TBR=hta@webrtc.org ========== to ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ==========
https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:79: // This is from the H264 spec. On 2016/11/03 14:04:32, hta-webrtc wrote: > Please add the formal reference - ITU.T H.264(year) section <whatever>. > These specs are humongous, and have a nasty tendency to change over time. Done. https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:66: // return no level if not even level 1 is supported. On 2016/11/03 14:04:32, hta-webrtc wrote: > This can be made clearer. > Something like: > > Given that a decoder supports up to a given frame size (in pixels) at up to a > given number of frames per second, return the highest H.264 level where it can > guarantee that it will be able to support all valid encoded streams that are > within that level. > > For an encoder, I think the logic will be different - you would want the level > that would guarantee that if the other end agreed to handle it, you could drive > the encoder to maximum performance without overrunning the decoder. > > Thus, if 1280*720@30 is slightly more than Level 3.1 but less than level 4.1: > > SupportedDecoderLevel(1280*720, 30) = Level3.1 > SupportedEncoderLevel(1280*720, 30) = Level4.1 You are right about this and I thought about adding two versions when I implemented this named LowerBoundLevel/UpperBoundLevel or SupportedLevel/RequiredLevel. However, I don't think the SupportedEncoderLevel variant will ever be needed since we should base our offered level solely on decode level. We offer our decode level in SDP, and we limit the input to the encoder based on remote decode level. https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id_unittest.cc (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:83: EXPECT_FALSE(SupportedLevel(183 * 137, 30)); On 2016/11/03 14:04:32, hta-webrtc wrote: > Please add a comment as to why these resolution/frame combinations are unable to > support any level. Done.
https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2470133002/diff/40001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:66: // return no level if not even level 1 is supported. On 2016/11/03 14:04:32, hta-webrtc wrote: > This can be made clearer. > Something like: > > Given that a decoder supports up to a given frame size (in pixels) at up to a > given number of frames per second, return the highest H.264 level where it can > guarantee that it will be able to support all valid encoded streams that are > within that level. > > For an encoder, I think the logic will be different - you would want the level > that would guarantee that if the other end agreed to handle it, you could drive > the encoder to maximum performance without overrunning the decoder. > > Thus, if 1280*720@30 is slightly more than Level 3.1 but less than level 4.1: > > SupportedDecoderLevel(1280*720, 30) = Level3.1 > SupportedEncoderLevel(1280*720, 30) = Level4.1 I updated the comment according to your suggestion though.
Harald - ping.
lgtm https://codereview.webrtc.org/2470133002/diff/50004/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2470133002/diff/50004/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:67: rtc::Optional<Level> SupportedLevel(int max_frame_pixel_count, int max_fps); Suggest using a float or double for max_fps. The conversion between the two forms is hurtful in the codebase, and whenever we get content at 29.94 FPS, odd stuff happens.
https://codereview.webrtc.org/2470133002/diff/50004/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2470133002/diff/50004/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:67: rtc::Optional<Level> SupportedLevel(int max_frame_pixel_count, int max_fps); On 2016/11/08 09:21:16, hta-webrtc wrote: > Suggest using a float or double for max_fps. The conversion between the two > forms is hurtful in the codebase, and whenever we get content at 29.94 FPS, odd > stuff happens. Done, changed to float.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2470133002/#ps90001 (title: "Change fps type to float")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ========== to ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 ========== to ========== Add function for getting supported H264 level from max resolution and fps BUG=webrtc:6337 Committed: https://crrev.com/a92704e6f5e240b4656d58a39c10267a8deaaff0 Cr-Commit-Position: refs/heads/master@{#14969} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a92704e6f5e240b4656d58a39c10267a8deaaff0 Cr-Commit-Position: refs/heads/master@{#14969} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
