Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880963002:
Generate FMTP parameters for the H.264 codec. (Closed)
|
Created:
4 years, 8 months ago by hta-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, nisse-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionGenerate FMTP parameters for the H.264 codec.
This CL generates FMTP parameters that allow H.264 interoperation
with Firefox for the default codec list.
BUG=chromium:591971
Committed: https://crrev.com/a6b99448eec51527eca0bc59f6da71061d02e807
Cr-Commit-Position: refs/heads/master@{#12333}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Review comments addressed #
Total comments: 2
Patch Set 3 : Add another TODO #
Total comments: 6
Messages
Total messages: 29 (11 generated)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880963002/1
Description was changed from ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 ========== to ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 ==========
hta@webrtc.org changed reviewers: + hbos@webrtc.org, tommi@webrtc.org
This works. It's not pretty. TODOs added for how things should be done. Adding nisse to CC list for comments on videoengine effects.
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
https://codereview.webrtc.org/1880963002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:391: // TODO(hta): Move selection of profile-level-id to codec implementation. This is probably not the right place for these defaults. If no configurability is needed for now, push them down to the codec? Or else, pass in values from above, and move the default values there?
https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc#newco... webrtc/api/webrtcsdp.cc:1597: if (_stricmp(name.c_str(), kFmtpParams[i]) == 0) { nit: since you're here, can you change this to if (name.compare(kFmtpParams[i]) == 0) { https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest.cc File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2094: if (webrtc::H264Encoder::IsSupported()) { nit: change to if (!webrtc::H264Encoder::IsSupported()) return; https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2116: // Provoke an error to verify that test is run. I'm not groking this comment... is it a todo?
https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest.cc File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2101: size_t after_pt = message.find(" H264/90000"); Can we also make sure that the m=video line contains the H264 ID? https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2116: // Provoke an error to verify that test is run. On 2016/04/12 10:10:26, tommi-webrtc wrote: > I'm not groking this comment... is it a todo? Old comment that should be removed? https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2117: } Optional: How about logging if the test was skipped due to H264 not being supported to avoid false positives?
https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc#newco... webrtc/api/webrtcsdp.cc:1597: if (_stricmp(name.c_str(), kFmtpParams[i]) == 0) { On 2016/04/12 10:10:26, tommi-webrtc wrote: > nit: since you're here, can you change this to > > if (name.compare(kFmtpParams[i]) == 0) { But i guess .compare is case sensitive, while _stricmp isn't?
Easily addressable comments addressed. Yes, the relationship between parameters in cricket::VideoCodec and the properties of the encoder implementation is not how it should be. Refactoring needed, but not in this CL. https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp.cc#newco... webrtc/api/webrtcsdp.cc:1597: if (_stricmp(name.c_str(), kFmtpParams[i]) == 0) { On 2016/04/12 10:52:08, nisse-webrtc wrote: > On 2016/04/12 10:10:26, tommi-webrtc wrote: > > nit: since you're here, can you change this to > > > > if (name.compare(kFmtpParams[i]) == 0) { > > But i guess .compare is case sensitive, while _stricmp isn't? RFC 4566: An SDP session description consists of a number of lines of text of the form: <type>=<value> where <type> MUST be exactly one case-significant character and <value> is structured text whose format depends on <type>. In general, <value> is either a number of fields delimited by a single space character or a free format string, and is case-significant unless a specific field defines otherwise. I think .compare is correct (and the caller is in our own code). Changing. https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest.cc File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2101: size_t after_pt = message.find(" H264/90000"); On 2016/04/12 10:14:23, hbos wrote: > Can we also make sure that the m=video line contains the H264 ID? Argh. Can do this, but now we're passing the level where it's really proper to abandon the find/substr method and parse the SDP. Adding a TODO. https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2116: // Provoke an error to verify that test is run. On 2016/04/12 10:14:23, hbos wrote: > On 2016/04/12 10:10:26, tommi-webrtc wrote: > > I'm not groking this comment... is it a todo? > > Old comment that should be removed? Yep. Done. https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2117: } On 2016/04/12 10:14:23, hbos wrote: > Optional: How about logging if the test was skipped due to H264 not being > supported to avoid false positives? Will only have any effect if people actually look at the logfile. Skeptical. Advice? https://codereview.webrtc.org/1880963002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:391: // TODO(hta): Move selection of profile-level-id to codec implementation. On 2016/04/12 10:03:42, nisse-webrtc wrote: > This is probably not the right place for these defaults. If no configurability > is needed for now, push them down to the codec? Or else, pass in values from > above, and move the default values there? I couldn't find a better place for now. The classes below are VideoCodec() and Codec(). Neither is appropriate for codec-specific functionality. I was aghast to find that the Codec() class even allows changing the name of the codec - and that there is code that depends on that. There is no "above" that knows about types of codecs - this function is called DefaultVideoCodecList. It's more proper to bind this info to the implementation, but the VideoCodec class doesn't point to the implementation, and webrtcsdp.cc doesn't know about the classes that contain the implementation, as far as I could tell. As I said in the CL description - this patch is not pretty. Refactoring needed.
lgtm
lgtm https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest.cc File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1880963002/diff/1/webrtc/api/webrtcsdp_unittest... webrtc/api/webrtcsdp_unittest.cc:2101: size_t after_pt = message.find(" H264/90000"); On 2016/04/12 11:21:37, hta-webrtc wrote: > On 2016/04/12 10:14:23, hbos wrote: > > Can we also make sure that the m=video line contains the H264 ID? > > Argh. Can do this, but now we're passing the level where it's really proper to > abandon the find/substr method and parse the SDP. > Adding a TODO. Acknowledged. https://codereview.webrtc.org/1880963002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:388: if (CodecIsInternallySupported(kH264CodecName)) { Hmm, I guess this was always the case, but this conditional means the SDP will only be correctly offered if there are internal H.264 implementations available. Even if an external factory (Chrome) provides both HW encoder and HW decoder, an internal implementation must be available - and in that case unused - for the SDP to contain H264. I suppose it's correct behavior for the "default codec list" to only consider "default" (internal) codecs. But how is the case handled for purely external codecs, so that those codecs are offered in the SDP? If that case isn't handled, there must be an an implicit assumption somewhere about the default codec list being complete when offering SDP. Or, if that case is handled, perhaps there is another place in the code where we should modify the codec list. - Should we add a TODO before landing?
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1880963002/#ps40001 (title: "Add another TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880963002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880963002/40001
Thanks. CQ started. https://codereview.webrtc.org/1880963002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:388: if (CodecIsInternallySupported(kH264CodecName)) { On 2016/04/12 11:41:14, hbos wrote: > Hmm, I guess this was always the case, but this conditional means the SDP will > only be correctly offered if there are internal H.264 implementations available. > Even if an external factory (Chrome) provides both HW encoder and HW decoder, an > internal implementation must be available - and in that case unused - for the > SDP to contain H264. > > I suppose it's correct behavior for the "default codec list" to only consider > "default" (internal) codecs. But how is the case handled for purely external > codecs, so that those codecs are offered in the SDP? If that case isn't handled, > there must be an an implicit assumption somewhere about the default codec list > being complete when offering SDP. Or, if that case is handled, perhaps there is > another place in the code where we should modify the codec list. > > - Should we add a TODO before landing? Yes, that makes the third TODO here: Move SDP parameter generation for codecs to depend on the codec implementation, not on this file. We have another effort (somewhere) to remove the idea of "internal codecs" altogether and only have a "default codec list" that contains the codecs that are included in the compilation. That will hopefully revisit this piece of code!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hta@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880963002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880963002/40001
Message was sent while issue was closed.
Description was changed from ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 ========== to ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 ========== to ========== Generate FMTP parameters for the H.264 codec. This CL generates FMTP parameters that allow H.264 interoperation with Firefox for the default codec list. BUG=chromium:591971 Committed: https://crrev.com/a6b99448eec51527eca0bc59f6da71061d02e807 Cr-Commit-Position: refs/heads/master@{#12333} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a6b99448eec51527eca0bc59f6da71061d02e807 Cr-Commit-Position: refs/heads/master@{#12333}
Message was sent while issue was closed.
juberti@chromium.org changed reviewers: + juberti@chromium.org, pthatcher@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1597: if (name.compare(kFmtpParams[i]) == 0) { These should be case insensitive comparisons. This looks like a bug. https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... File webrtc/media/base/mediaconstants.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... webrtc/media/base/mediaconstants.cc:120: const char kH264FmtpProfileLevelId[] = "profile-level-id"; The precedent here is kCodecParamFoo, or perhaps in this case, kH264Param foo (see the opus params above). https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:398: codec.SetParam(kH264FmtpLevelAsymmetryAllowed, "1"); You can use the int version here, e.g. 1 instead of "1".
Message was sent while issue was closed.
Pushback on 2 of the comments. As I said, FMTP parameter setting need to be refactored to be implementation-driven, so this all needs to be revisited soon. Not planning to reopen. https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:1597: if (name.compare(kFmtpParams[i]) == 0) { On 2016/04/13 20:20:58, juberti2 wrote: > These should be case insensitive comparisons. This looks like a bug. Citation? I checked the SDP RFC, and it said "case sensitive by default". https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... File webrtc/media/base/mediaconstants.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... webrtc/media/base/mediaconstants.cc:120: const char kH264FmtpProfileLevelId[] = "profile-level-id"; On 2016/04/13 20:20:58, juberti2 wrote: > The precedent here is kCodecParamFoo, or perhaps in this case, kH264Param foo > (see the opus params above). The precedent is wrong. The namespace for FMTP parameters is scoped by codec, mixing the names of FMTP codec parameters like these with the names of codec parameters that go on a= lines is going to cause confusion. https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:398: codec.SetParam(kH264FmtpLevelAsymmetryAllowed, "1"); On 2016/04/13 20:20:58, juberti2 wrote: > You can use the int version here, e.g. 1 instead of "1". Acknowledged.
Message was sent while issue was closed.
On 2016/04/13 20:43:58, hta-webrtc wrote: > Pushback on 2 of the comments. As I said, FMTP parameter setting need to be > refactored to be implementation-driven, so this all needs to be revisited soon. > > Not planning to reopen. > > https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc > File webrtc/api/webrtcsdp.cc (right): > > https://codereview.webrtc.org/1880963002/diff/40001/webrtc/api/webrtcsdp.cc#n... > webrtc/api/webrtcsdp.cc:1597: if (name.compare(kFmtpParams[i]) == 0) { > On 2016/04/13 20:20:58, juberti2 wrote: > > These should be case insensitive comparisons. This looks like a bug. > > Citation? I checked the SDP RFC, and it said "case sensitive by default". RFC 3264 is indeed ambiguous, but 4855 makes a clear statement: Note that the payload format (encoding) names defined in the RTP Profile [4] are commonly shown in upper case. Media subtype names are commonly shown in lower case. These names are case-insensitive in both places. Similarly, parameter names are case-insensitive both in media type strings and in the default mapping to the SDP a=fmtp attribute. > > https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... > File webrtc/media/base/mediaconstants.cc (right): > > https://codereview.webrtc.org/1880963002/diff/40001/webrtc/media/base/mediaco... > webrtc/media/base/mediaconstants.cc:120: const char kH264FmtpProfileLevelId[] = > "profile-level-id"; > On 2016/04/13 20:20:58, juberti2 wrote: > > The precedent here is kCodecParamFoo, or perhaps in this case, kH264Param foo > > (see the opus params above). > > The precedent is wrong. The namespace for FMTP parameters is scoped by codec, > mixing the names of FMTP codec parameters like these with the names of codec > parameters that go on a= lines is going to cause confusion. The Opus parameters are akin to these parameters (i.e. they go into fmtp); I don't think there will be confusion on where these parameters go. Overall the kCodecParam precedent was chosen based on 4855, which allows codecs to register arbitrary params which can be serialized in different ways, including fmtp. I agree that these parameters are codec specific, so if we think kCodecParamFoo is potentially confusing, we could go with kOpusParamFoo/kH264ParamFoo instead. The overall point is that we don't use fmtp terminology anywhere else, so we shouldn't start using it here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
