Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1896983004:
Rename misleading |is_ambiguous| parameter
DescriptionRename misleading |is_ambiguous| parameter
TEST=Existing MediaCanPlayTypeTests
Patch Set 1 #
Total comments: 24
Patch Set 2 : review feedback #Patch Set 3 : rebase #Patch Set 4 : update VP9 comment to match #
Total comments: 2
Patch Set 5 : fix comment: VP8 is still handled by the map #Patch Set 6 : rename is_known_supported to is_probably_supported #Patch Set 7 : Rebase on b36186ff03599c360e1c536f968aa7022105d3d0. #Patch Set 8 : Update out of date comment #Messages
Total messages: 18 (6 generated)
ddorwin@chromium.org changed reviewers: + xhwang@chromium.org
Naming suggestions very welcome. I considered |is_confident...| and |is_likely_supported|. Note the inversion in the parameter's meaning. The current naming has been a point of confusion recently: * https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... * https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i... https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (left): https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:170: // TODO(servolk): Most HEVC codec ids are treated as ambiguous (see above), This should have been removed in https://codereview.chromium.org/1864743002/. https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:35: // hev1/hvc1.XXXXXX is handled by ParseHEVCCodecID(). Oops - fixed locally. https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.h:101: // Returns IsSupported if all codec IDs in |codecs| are unambiguous, known to "known to be supported" here, which is not "confidently known to be supported" below, is the most problematic naming issue.
The only truly ambiguous codec strings now are those few in kAmbiguousCodecStringMap.
On 2016/04/18 23:45:06, ddorwin wrote: > The only truly ambiguous codec strings now are those few in > kAmbiguousCodecStringMap. |is_ambiguous| was added in https://codereview.chromium.org/422573005/diff/120001/net/base/mime_util.cc with the same set of strings in kAmbiguousCodecIDs. I guess what has changed is that we now ParseH264CodecID() (now ParseAVCCodecId()) has become a lot more detailed/strict so there are less "unknown" values. Even then, "ambiguous" may not have been correct, but it was consistent with the use related to kAmbiguousCodecIDs, which was added at the same time.
Description was changed from ========== Rename misleading |is_ambiguous| parameter TEST=Existing MediaCanPlayTypeTests ========== to ========== Rename misleading |is_ambiguous| parameter TEST=Existing MediaCanPlayTypeTests ==========
ddorwin@chromium.org changed reviewers: + dalecurtis@chromium.org - xhwang@chromium.org
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Mostly nits and naming. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... File media/base/mime_util_internal.h (right): https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:86: bool is_ambiguous; Do you want rename this as well? https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:102: // be supported, and are supported in |mime_type_lower_case|. MayBeSupported nit and not for this CL: Since "maybe" is one word, this should be MaybeSupported? https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:108: SupportsType AreSupportedCodecs(const CodecSet& supported_codecs, What is this parameter for? It's not documented and is not obvious. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:113: // Converts a codec ID into an Codec enum value and indicates nit: s/an/a also needs reflow https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:114: // whether the specified codec, including profile, etc., is is known supported s/is is/is/ https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:122: bool* is_known_supported, Thanks for the background context on naming. So is |is_known_supported| strictly the opposite of |is_ambiguous|? If so, can this just be |is_supported|, which is "unambiguously" supported? It may seems odd in MimeUtil::AreSupportedCodecs() when we check Is*Supported twice. But I don't really feel |is_known_supported| is clearer. Or, can we have a comment here about what |is_known_supported| exactly means?
https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... File media/base/mime_util_internal.h (right): https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:101: // Returns IsSupported if all codec IDs in |codecs| are unambiguous, known to On 2016/04/18 at 23:43:37, ddorwin wrote: > "known to be supported" here, which is not "confidently known to be supported" below, is the most problematic naming issue. Yeah, this is just redundant. It doesn't make sense to have both "known to be supported" and "are supported". Replace one or the other with your preferred wording. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:104: // |codecs| is ambiguous or not confidently known to be supported. maybe something like "support can not be determined at this time" ?
Patchset #2 (id:20001) has been deleted
PS2 addresses the feedback. PS3 (and PS4) rebases on top of https://codereview.chromium.org/1624703002/, including inverting the logic added in that CL. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... File media/base/mime_util_internal.cc (right): https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.cc:35: // hev1/hvc1.XXXXXX is handled by ParseHEVCCodecID(). On 2016/04/18 23:43:37, ddorwin wrote: > Oops - fixed locally. Done. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... File media/base/mime_util_internal.h (right): https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:86: bool is_ambiguous; On 2016/04/19 23:59:43, xhwang wrote: > Do you want rename this as well? Hmm. It is set for ambiguous strings (kAmbiguousCodecStringMap). It's just a matter of where we do the conversion from ambiguous to not known supported. Currently, that is in StringToCodec(). To the extent that this is an object representation of the static tables, I think it probably makes sense to leave it this way, but I don't feel strongly. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:101: // Returns IsSupported if all codec IDs in |codecs| are unambiguous, known to On 2016/04/20 19:18:54, DaleCurtis wrote: > On 2016/04/18 at 23:43:37, ddorwin wrote: > > "known to be supported" here, which is not "confidently known to be supported" > below, is the most problematic naming issue. > > Yeah, this is just redundant. It doesn't make sense to have both "known to be > supported" and "are supported". Replace one or the other with your preferred > wording. It turns out this was a bit of a bad example. I've fixed the comments. We now have: 1. "confidently known to be supported" - a codec string we fully recognize and "know" we support 2. "supported in |mime_type_lower_case|" - a valid codec for the container. We still have the larger question of how to describe #1. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:102: // be supported, and are supported in |mime_type_lower_case|. MayBeSupported On 2016/04/19 23:59:42, xhwang wrote: > nit and not for this CL: Since "maybe" is one word, this should be > MaybeSupported? I believe this is based on "the codec may be supported" just like "the codec is [not] supported." The single word would be something like "Maybe the codec is supported." https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:104: // |codecs| is ambiguous or not confidently known to be supported. On 2016/04/20 19:18:54, DaleCurtis wrote: > maybe something like "support can not be determined at this time" ? Reworded. WDYT? https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:108: SupportsType AreSupportedCodecs(const CodecSet& supported_codecs, On 2016/04/19 23:59:42, xhwang wrote: > What is this parameter for? It's not documented and is not obvious. Done. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:113: // Converts a codec ID into an Codec enum value and indicates On 2016/04/19 23:59:42, xhwang wrote: > nit: s/an/a Done. > > also needs reflow Done. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:114: // whether the specified codec, including profile, etc., is is known supported On 2016/04/19 23:59:42, xhwang wrote: > s/is is/is/ Done. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:122: bool* is_known_supported, On 2016/04/19 23:59:42, xhwang wrote: > Thanks for the background context on naming. So is |is_known_supported| strictly > the opposite of |is_ambiguous|? If so, can this just be |is_supported|, which is > "unambiguously" supported? It may seems odd in MimeUtil::AreSupportedCodecs() > when we check Is*Supported twice. But I don't really feel |is_known_supported| > is clearer. > > Or, can we have a comment here about what |is_known_supported| exactly means? No, it is not strictly the opposite. Ambiguous (now) means the string does not give us enough information to determine whether we support it. As I said above, this is now just the three strings in kAmbiguousCodecStringMap. Ambiguous is a subset of things that are not |is_known_supported|. There are others, including values we don't support (i.e. AVC extended profile). Most relevant to this discussion, there are things like Hi10p, which may be supported but we don't actually know (except for FFmpeg in this specific case). I agree that we should have a clearer name. Maybe |is_probably_supported|, which is consistent with "probably". It's also similar to "likely", which was one of my suggestions. I've updated the comment. I'll hold off on renaming until we pick a name.
lgtm with the current wording.
kqyang@chromium.org changed reviewers: + kqyang@chromium.org
https://chromiumcodereview.appspot.com/1896983004/diff/80001/media/base/mime_... File media/base/mime_util_internal.cc (right): https://chromiumcodereview.appspot.com/1896983004/diff/80001/media/base/mime_... media/base/mime_util_internal.cc:36: // vp8, vp8.0, vp9, vp9.0, and vp09.xx.xx.xx.xx.xx.xx.xx are handled by vp8, vp8.0 are still handled in this map; only vp9, vp9.0 and vp09 are handled in ParseVp9CodecID.
https://chromiumcodereview.appspot.com/1896983004/diff/80001/media/base/mime_... File media/base/mime_util_internal.cc (right): https://chromiumcodereview.appspot.com/1896983004/diff/80001/media/base/mime_... media/base/mime_util_internal.cc:36: // vp8, vp8.0, vp9, vp9.0, and vp09.xx.xx.xx.xx.xx.xx.xx are handled by On 2016/04/21 00:59:48, kqyang wrote: > vp8, vp8.0 are still handled in this map; only vp9, vp9.0 and vp09 are handled > in ParseVp9CodecID. Done. Thanks.
lgtm % potential naming change https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... File media/base/mime_util_internal.h (right): https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:102: // be supported, and are supported in |mime_type_lower_case|. MayBeSupported On 2016/04/21 00:50:30, ddorwin wrote: > On 2016/04/19 23:59:42, xhwang wrote: > > nit and not for this CL: Since "maybe" is one word, this should be > > MaybeSupported? > > I believe this is based on "the codec may be supported" just like "the codec is > [not] supported." > > The single word would be something like "Maybe the codec is supported." Acknowledged. https://chromiumcodereview.appspot.com/1896983004/diff/1/media/base/mime_util... media/base/mime_util_internal.h:122: bool* is_known_supported, On 2016/04/21 00:50:30, ddorwin wrote: > On 2016/04/19 23:59:42, xhwang wrote: > > Thanks for the background context on naming. So is |is_known_supported| > strictly > > the opposite of |is_ambiguous|? If so, can this just be |is_supported|, which > is > > "unambiguously" supported? It may seems odd in MimeUtil::AreSupportedCodecs() > > when we check Is*Supported twice. But I don't really feel |is_known_supported| > > is clearer. > > > > Or, can we have a comment here about what |is_known_supported| exactly means? > > No, it is not strictly the opposite. Ambiguous (now) means the string does not > give us enough information to determine whether we support it. As I said above, > this is now just the three strings in kAmbiguousCodecStringMap. > > Ambiguous is a subset of things that are not |is_known_supported|. There are > others, including values we don't support (i.e. AVC extended profile). Most > relevant to this discussion, there are things like Hi10p, which may be supported > but we don't actually know (except for FFmpeg in this specific case). > > I agree that we should have a clearer name. Maybe |is_probably_supported|, which > is consistent with "probably". It's also similar to "likely", which was one of > my suggestions. > > I've updated the comment. I'll hold off on renaming until we pick a name. |is_probably_supported| is probably better, but not known better :) Joke aside, I do feel |is_known_supported| is a bit too strong for what you described. |is_probably_supported| does sound better to me.
I will rebase and land after https://codereview.chromium.org/1677133003/ lands. https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.h:122: bool* is_known_supported, On 2016/04/21 05:59:44, xhwang wrote: > On 2016/04/21 00:50:30, ddorwin wrote: > > On 2016/04/19 23:59:42, xhwang wrote: > > > Thanks for the background context on naming. So is |is_known_supported| > > strictly > > > the opposite of |is_ambiguous|? If so, can this just be |is_supported|, > which > > is > > > "unambiguously" supported? It may seems odd in > MimeUtil::AreSupportedCodecs() > > > when we check Is*Supported twice. But I don't really feel > |is_known_supported| > > > is clearer. > > > > > > Or, can we have a comment here about what |is_known_supported| exactly > means? > > > > No, it is not strictly the opposite. Ambiguous (now) means the string does not > > give us enough information to determine whether we support it. As I said > above, > > this is now just the three strings in kAmbiguousCodecStringMap. > > > > Ambiguous is a subset of things that are not |is_known_supported|. There are > > others, including values we don't support (i.e. AVC extended profile). Most > > relevant to this discussion, there are things like Hi10p, which may be > supported > > but we don't actually know (except for FFmpeg in this specific case). > > > > I agree that we should have a clearer name. Maybe |is_probably_supported|, > which > > is consistent with "probably". It's also similar to "likely", which was one of > > my suggestions. > > > > I've updated the comment. I'll hold off on renaming until we pick a name. > > |is_probably_supported| is probably better, but not known better :) Joke aside, > I do feel |is_known_supported| is a bit too strong for what you described. > |is_probably_supported| does sound better to me. Done.
StringToCodec() is a bit confusing and inconsistent with respect to proprietary codecs. For both those in string_to_codec_map_ and H.264, we always identify the codec and return true regardless of whether proprietary codecs are enabled. (For HEVC, we don't parse at all.) We then rely on a subsequent call to IsCodecSupported to reject proprietary codecs. This was a bit strange but perhaps fine until we added an assertive statement: |is_probably_supported|. Thoughts? Should we revert to a negative meaning, such as |support_cannot_be_determined|? Even then, it seems a bit misleading to imply "maybe this proprietary codec is supported" when we know it's not. Really, we should make these decisions outside the string conversion function, but then we'd have to expose more output parameters and/or callback(s). (This is something we may need to do in order to detect profiles, etc., but that's overkill for now.) https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (left): https://codereview.chromium.org/1896983004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:170: // TODO(servolk): Most HEVC codec ids are treated as ambiguous (see above), On 2016/04/18 23:43:37, ddorwin wrote: > This should have been removed in https://codereview.chromium.org/1864743002/. But, it was still affecting the output, which these tests were catching on Cast. This is moot now that https://codereview.chromium.org/1677133003 has landed. |
