Implemented parsing of HEVC codec ids

media/base/video_codecs.h

Index: media/base/video_codecs.h
diff --git a/media/base/video_codecs.h b/media/base/video_codecs.h
index 188b5449f7906ccb5485122ef6f75563ddde4c0b..33e234d07b97aaabe8cd8e4eaaec8883d36d5eee 100644
--- a/media/base/video_codecs.h
+++ b/media/base/video_codecs.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 #include <string>
 #include "media/base/media_export.h"
+#include "media/media_features.h"
 
 namespace media {
 
@@ -54,10 +55,15 @@ enum VideoCodecProfile {
   H264PROFILE_STEREOHIGH = 9,
   H264PROFILE_MULTIVIEWHIGH = 10,
   H264PROFILE_MAX = H264PROFILE_MULTIVIEWHIGH,
-  VP8PROFILE_MIN = 11,
+  HEVCPROFILE_MAIN,

[ddorwin, 2016/02/18 17:30:02]

All other codecs follow this format:
MIN
FOO=MIN
BAR
MAX=BAR

[servolk, 2016/02/18 19:16:20]

Done (I've also removed unnecessary explicit assignment of values for individual
enum members, as currently changing this enum is quite painful, I think we
should be ok with having static asserts to ensure the media::VideoCodecProfile
enum values match mojo and gpu enums).

[ddorwin, 2016/02/18 21:52:22]

+dalecurtis to confirm there are not any problems with this.

[DaleCurtis, 2016/02/18 22:04:55]

You can't do this since these are logged to UMA, they must be put on the end.
They should also be protected by the build flag.

[ddorwin, 2016/02/18 22:21:41]

Ahh, thanks. I checked one of the other enums for the histogram note and didn't
see it, but it is here (line 16). We should make it more prominent.

Having these MIN and MAX values in an enum we can't change is problematic. If we
decided to add an H264 profile or HEVC profile (as it sounds like servolk
intends to in his comment below), we could not add it. We may need to refactor
out these MIN/MAX and find another way to determine this.

[servolk, 2016/02/18 22:28:51]

Ok, I'll move the HEVC profiles after VP8/VP9, but are we absolutely sure that
there's never gonna be more than one vp8/vp9 profile? If they ever introduce the
concept of profiles, are we going to have them  interspersed in this enum?

[DaleCurtis, 2016/02/19 23:15:17]

Doesn't really matter if we're sure or not; these values are already logged to a
histogram, so unless we want to create a new UMA stat, old values must remain
the same.

[servolk, 2016/02/19 23:47:55]

Done (I've also restored the explicit value numbering in this enum, to ensure
values don't ever change).
Re guarding with a build flag - I think I've been told earlier that values in
the enum that are reported to UMA shouldn't be guarded (e.g. take a look at
VideoCodec::kCodecHEVC above - it's not protected by the buildflag).

[servolk, 2016/02/19 23:57:31]

Btw, I've also just noticed that there's a mismatch between these values and
PPAPI values (despite the comments at lines 36-38 above).
https://bugs.chromium.org/p/chromium/issues/detail?id=588336

+  HEVCPROFILE_MIN = HEVCPROFILE_MAIN,
+  HEVCPROFILE_MAIN10,
+  HEVCPROFILE_MAIN_STILL_PICTURE,

[ddorwin, 2016/02/18 17:30:02]

This is just an image right? Do you have plans to support it? Would it be
supported in <video> or <img>?

[servolk, 2016/02/18 19:16:20]

Yes, this is a single video frame, i.e. essentially just an image, only encoded
with the same compression techniques as video. According to the HEVC standard
the 'main still picture' profile is a subset of the main profile, so at least in
theory it should be automatically supported by the video element if it supports
the main HEVC video profile. There's no immediate plans to support that for
<img>. I'm mainly adding it here because 'main still picture' is one of the
three v1 general profiles described in HEVC standard
(https://www.itu.int/rec/T-REC-H.265) section A.3.1
There is also a bunch of format range extensions profiles described in  section
A.3.5, but I haven't yet looked into how to support those best, while this one
is straightforward.

+  HEVCPROFILE_MAX = HEVCPROFILE_MAIN_STILL_PICTURE,
+  VP8PROFILE_MIN,
   VP8PROFILE_ANY = VP8PROFILE_MIN,
   VP8PROFILE_MAX = VP8PROFILE_ANY,
-  VP9PROFILE_MIN = 12,
+  VP9PROFILE_MIN,
   VP9PROFILE_ANY = VP9PROFILE_MIN,
   VP9PROFILE_MAX = VP9PROFILE_ANY,
   VIDEO_CODEC_PROFILE_MAX = VP9PROFILE_MAX,
@@ -71,6 +77,12 @@ MEDIA_EXPORT bool ParseAVCCodecId(const std::string& codec_id,
                                   VideoCodecProfile* profile,
                                   uint8_t* level_idc);
 
+#if BUILDFLAG(ENABLE_HEVC_DEMUXING)
+MEDIA_EXPORT bool ParseHEVCCodecId(const std::string& codec_id,

[ddorwin, 2016/02/18 17:30:03]

Why have these been moved from mime_util and exposed not just outside the
internal.cc file but outside media.lib?

[servolk, 2016/02/18 19:16:20]

I've exposed them as media_export to make them accessible to third-party vendor
code in chromecast. Re moving from mime_util - this file seemed like a more
appropriate place for this, since it defines various video codec profiles and
also deals with video codecs/profiles and their string representations. But I
can move the parsing functions back to mime_util, if you think that would be a
better place for this code.

[ddorwin, 2016/02/18 21:52:22]

I'd like to understand more about that usage - not necessarily here. As
previously discussed, having one parser is better than multiple, but my initial
reactions is that:
a) We shouldn't be passing such strings to third-party code, so it shouldn't be
necessary for that.
b) If it is for other reasons, it's strange that vendor code would need to do
this and that they would rely on our code. I would also be concerned that they
might then come to rely on bugs in our code, making it harder for us to fix them
for all other platforms.

[servolk, 2016/02/18 22:28:51]

Sure, we can probably move that discussion to a separate mail thread. But in
short: currently Chromecast codec checks are done via string codec ids since, as
I've explained before, the enums are simply not enough to express the richness
of the information encoded in codec id string (all the various
profiles/levels/tiers/etc). We are planning to keep working with you on getting
to a common upstreamable solution that satisfies everybody, but we needed those
codec checks to be done sooner rather than later, so I've created a couple of
downstream CLs to unblock Chromecast development.

+                                   VideoCodecProfile* profile,
+                                   uint8_t* level_idc);
+#endif
+
 }  // namespace media
 
 #endif  // MEDIA_BASE_VIDEO_CODECS_H_