Refactoring media mime type and supported codec checks

Refactoring media mime type and supported codec checks

BUG=477103

[servolk, 2015-04-14 22:32:28]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, ddorwin@chromium.org, erickung@chromium.org,
gunsch@chromium.org, lcwu@chromium.org, rsleevi@chromium.org,
sandersd@chromium.org, wolenetz@chromium.org

[servolk, 2015-04-14 22:49:30]

PTAL

[DaleCurtis, 2015-04-15 22:44:31]

dalecurtis@chromium.org changed reviewers:
- sandersd@chromium.org, wolenetz@chromium.org

[DaleCurtis, 2015-04-15 22:44:31]

I worry about duplicating these strings everywhere; which I think was the intent
of using enum values in the existing architecture. Practically I'm not sure it
helped since the existing code has them duplicated in just as many places :-/

In that vein, we have http://crbug.com/461009 (mentioned in the comments you are
deleting) which suggests we want to move away from strings instead of towards
them.

I'm not familiar with the history here, but
https://codereview.chromium.org/422573005 is the CL which switched from strings
to enum values as you see today.

I don't have strong feelings here, but I suspect ddorwin@ does. I've moved a
couple of reviewers to cc as such.

https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc#...
net/base/mime_util.cc:439: return DefaultIsCodecSupported(codec);
Did you run these tests on Android? I'd be surprised if this works...

[servolk, 2015-04-15 23:23:17]

On 2015/04/15 22:44:31, DaleCurtis wrote:
> I worry about duplicating these strings everywhere; which I think was the
intent
> of using enum values in the existing architecture. Practically I'm not sure it
> helped since the existing code has them duplicated in just as many places :-/
> 
> In that vein, we have http://crbug.com/461009 (mentioned in the comments you
are
> deleting) which suggests we want to move away from strings instead of towards
> them.
> 
> I'm not familiar with the history here, but
> https://codereview.chromium.org/422573005 is the CL which switched from
strings
> to enum values as you see today.
> 
> I don't have strong feelings here, but I suspect ddorwin@ does. I've moved a
> couple of reviewers to cc as such.
> 
> https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc#...
> net/base/mime_util.cc:439: return DefaultIsCodecSupported(codec);
> Did you run these tests on Android? I'd be surprised if this works...

Thanks for your comments and providing insight into history of this! I have
looked at https://codereview.chromium.org/422573005 and as far as I understand
the main intent of it was to make sure we properly return "maybe" for ambiguous
codec ids and "probably" for concrete codec ids that are known to be supported
for sure. Now fortunately that CL added a lot of new unit test cases and all of
those test cases pass after my change (yes, even including Android, if you look
at trybot results for patchset #9 there were 3 CanPlay test failures on
linux_android_rel_ng bot, but in patchset #10 and #11 these tests pass on
linux_android_rel_ng, since I have properly hooked up the
Android-platform-specific function in patchset #10).
I think one thing that the CL 422573005 demonstrated was that introducing enums
made the code bulkier (+358 lines, -187 lines for mime_util.cc) and yet it's not
clear to me if we got any other tangible advantages, since as you've mentioned,
yes, we still have to repeat all those enum values in a lot of places. I hope
you'll agree that my CL demonstrates that we can still achieve the same effect
by using strings instead of enum values. And as I've argued earlier I think
using strings makes our code more flexible. Let me reiterate what, I think, we
could get by using strings:
1. The codec_id as a string contains the complete information about the codec
whereas limited set of enum values forces us to lose some information about
codec level/profile (this is especially important for avc1/avc3 video codecs as
well as HEVC and other advanced video codecs, for which it's hard to encode the
complete codec info in a single enum, we'd have to use a struct like
VideoDecoderConfig)
2. It should make it easier to begin the unification of codec checks between
MSE/HTMLMediaElement.CanPlayType /EME code paths. The fact that IsCodecSupported
uses an internal enum value only accessible in net/base/mime_util.cc makes it
harder and if we exposed the enum externally, that would create some confusion -
which one to use MimeUtil::Codec or media::AudioCodec/media::VideoCodec.
3. It should make it easier to add new supported codecs in platform-specific
code. E.g. if Chromium doesn't care about HEVC, that's ok, we wouldn't even need
to mention it in Chromium code then. We could just handle HEVC codec ids in
platform-specific Android and Chromecast functions.

I was trying to think if we got any advantage by using enums, and the only thing
that comes to my mind is possibly a small gain in performance. But:
a. The codecs are initially provides as strings in the mime type anyway. So we
have to convert the codec id from string into enum first in any case and only
then do some checks with enums.
b. I don't expect this code to be very perf-sensitive. These kinds of checks are
typically done only once at the beginning of the playback, it's not like we are
checking mime type for every video frame, so I think we could safely sacrifice
some performance here in favor of flexibility and maintainability.

[servolk, 2015-04-16 02:04:45]

On 2015/04/15 23:23:17, servolk wrote:
> On 2015/04/15 22:44:31, DaleCurtis wrote:
> > I worry about duplicating these strings everywhere; which I think was the
> intent
> > of using enum values in the existing architecture. Practically I'm not sure
it
> > helped since the existing code has them duplicated in just as many places
:-/
> > 
> > In that vein, we have http://crbug.com/461009 (mentioned in the comments you
> are
> > deleting) which suggests we want to move away from strings instead of
towards
> > them.
> > 
> > I'm not familiar with the history here, but
> > https://codereview.chromium.org/422573005 is the CL which switched from
> strings
> > to enum values as you see today.
> > 
> > I don't have strong feelings here, but I suspect ddorwin@ does. I've moved a
> > couple of reviewers to cc as such.
> > 
> > https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc
> > File net/base/mime_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc#...
> > net/base/mime_util.cc:439: return DefaultIsCodecSupported(codec);
> > Did you run these tests on Android? I'd be surprised if this works...
> 
> Thanks for your comments and providing insight into history of this! I have
> looked at https://codereview.chromium.org/422573005 and as far as I understand
> the main intent of it was to make sure we properly return "maybe" for
ambiguous
> codec ids and "probably" for concrete codec ids that are known to be supported
> for sure. Now fortunately that CL added a lot of new unit test cases and all
of
> those test cases pass after my change (yes, even including Android, if you
look
> at trybot results for patchset #9 there were 3 CanPlay test failures on
> linux_android_rel_ng bot, but in patchset #10 and #11 these tests pass on
> linux_android_rel_ng, since I have properly hooked up the
> Android-platform-specific function in patchset #10).
> I think one thing that the CL 422573005 demonstrated was that introducing
enums
> made the code bulkier (+358 lines, -187 lines for mime_util.cc) and yet it's
not
> clear to me if we got any other tangible advantages, since as you've
mentioned,
> yes, we still have to repeat all those enum values in a lot of places. I hope
> you'll agree that my CL demonstrates that we can still achieve the same effect
> by using strings instead of enum values. And as I've argued earlier I think
> using strings makes our code more flexible. Let me reiterate what, I think, we
> could get by using strings:
> 1. The codec_id as a string contains the complete information about the codec
> whereas limited set of enum values forces us to lose some information about
> codec level/profile (this is especially important for avc1/avc3 video codecs
as
> well as HEVC and other advanced video codecs, for which it's hard to encode
the
> complete codec info in a single enum, we'd have to use a struct like
> VideoDecoderConfig)
> 2. It should make it easier to begin the unification of codec checks between
> MSE/HTMLMediaElement.CanPlayType /EME code paths. The fact that
IsCodecSupported
> uses an internal enum value only accessible in net/base/mime_util.cc makes it
> harder and if we exposed the enum externally, that would create some confusion
-
> which one to use MimeUtil::Codec or media::AudioCodec/media::VideoCodec.
> 3. It should make it easier to add new supported codecs in platform-specific
> code. E.g. if Chromium doesn't care about HEVC, that's ok, we wouldn't even
need
> to mention it in Chromium code then. We could just handle HEVC codec ids in
> platform-specific Android and Chromecast functions.
> 
> I was trying to think if we got any advantage by using enums, and the only
thing
> that comes to my mind is possibly a small gain in performance. But:
> a. The codecs are initially provides as strings in the mime type anyway. So we
> have to convert the codec id from string into enum first in any case and only
> then do some checks with enums.
> b. I don't expect this code to be very perf-sensitive. These kinds of checks
are
> typically done only once at the beginning of the playback, it's not like we
are
> checking mime type for every video frame, so I think we could safely sacrifice
> some performance here in favor of flexibility and maintainability.

Btw, to be fair, after thinking a bit more about this I can see one other
advantage of using enums: you get compile time checks that ensure, for example,
that you are handling all possible codec values in a switch. But that cuts two
ways - it also forces you to handle all the codecs in a single function and
makes it harder to support the "override" semantics where you can choose to make
explicit choices only about a few specific codecs in your platform-specific
function and let everything else to be decided by some default implementation.
So it limits API flexibility to some degree.

[servolk, 2015-04-20 20:53:20]

On 2015/04/16 02:04:45, servolk wrote:
> On 2015/04/15 23:23:17, servolk wrote:
> > On 2015/04/15 22:44:31, DaleCurtis wrote:
> > > I worry about duplicating these strings everywhere; which I think was the
> > intent
> > > of using enum values in the existing architecture. Practically I'm not
sure
> it
> > > helped since the existing code has them duplicated in just as many places
> :-/
> > > 
> > > In that vein, we have http://crbug.com/461009 (mentioned in the comments
you
> > are
> > > deleting) which suggests we want to move away from strings instead of
> towards
> > > them.
> > > 
> > > I'm not familiar with the history here, but
> > > https://codereview.chromium.org/422573005 is the CL which switched from
> > strings
> > > to enum values as you see today.
> > > 
> > > I don't have strong feelings here, but I suspect ddorwin@ does. I've moved
a
> > > couple of reviewers to cc as such.
> > > 
> > >
https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc
> > > File net/base/mime_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1054943004/diff/200001/net/base/mime_util.cc#...
> > > net/base/mime_util.cc:439: return DefaultIsCodecSupported(codec);
> > > Did you run these tests on Android? I'd be surprised if this works...
> > 
> > Thanks for your comments and providing insight into history of this! I have
> > looked at https://codereview.chromium.org/422573005 and as far as I
understand
> > the main intent of it was to make sure we properly return "maybe" for
> ambiguous
> > codec ids and "probably" for concrete codec ids that are known to be
supported
> > for sure. Now fortunately that CL added a lot of new unit test cases and all
> of
> > those test cases pass after my change (yes, even including Android, if you
> look
> > at trybot results for patchset #9 there were 3 CanPlay test failures on
> > linux_android_rel_ng bot, but in patchset #10 and #11 these tests pass on
> > linux_android_rel_ng, since I have properly hooked up the
> > Android-platform-specific function in patchset #10).
> > I think one thing that the CL 422573005 demonstrated was that introducing
> enums
> > made the code bulkier (+358 lines, -187 lines for mime_util.cc) and yet it's
> not
> > clear to me if we got any other tangible advantages, since as you've
> mentioned,
> > yes, we still have to repeat all those enum values in a lot of places. I
hope
> > you'll agree that my CL demonstrates that we can still achieve the same
effect
> > by using strings instead of enum values. And as I've argued earlier I think
> > using strings makes our code more flexible. Let me reiterate what, I think,
we
> > could get by using strings:
> > 1. The codec_id as a string contains the complete information about the
codec
> > whereas limited set of enum values forces us to lose some information about
> > codec level/profile (this is especially important for avc1/avc3 video codecs
> as
> > well as HEVC and other advanced video codecs, for which it's hard to encode
> the
> > complete codec info in a single enum, we'd have to use a struct like
> > VideoDecoderConfig)
> > 2. It should make it easier to begin the unification of codec checks between
> > MSE/HTMLMediaElement.CanPlayType /EME code paths. The fact that
> IsCodecSupported
> > uses an internal enum value only accessible in net/base/mime_util.cc makes
it
> > harder and if we exposed the enum externally, that would create some
confusion
> -
> > which one to use MimeUtil::Codec or media::AudioCodec/media::VideoCodec.
> > 3. It should make it easier to add new supported codecs in platform-specific
> > code. E.g. if Chromium doesn't care about HEVC, that's ok, we wouldn't even
> need
> > to mention it in Chromium code then. We could just handle HEVC codec ids in
> > platform-specific Android and Chromecast functions.
> > 
> > I was trying to think if we got any advantage by using enums, and the only
> thing
> > that comes to my mind is possibly a small gain in performance. But:
> > a. The codecs are initially provides as strings in the mime type anyway. So
we
> > have to convert the codec id from string into enum first in any case and
only
> > then do some checks with enums.
> > b. I don't expect this code to be very perf-sensitive. These kinds of checks
> are
> > typically done only once at the beginning of the playback, it's not like we
> are
> > checking mime type for every video frame, so I think we could safely
sacrifice
> > some performance here in favor of flexibility and maintainability.
> 
> Btw, to be fair, after thinking a bit more about this I can see one other
> advantage of using enums: you get compile time checks that ensure, for
example,
> that you are handling all possible codec values in a switch. But that cuts two
> ways - it also forces you to handle all the codecs in a single function and
> makes it harder to support the "override" semantics where you can choose to
make
> explicit choices only about a few specific codecs in your platform-specific
> function and let everything else to be decided by some default implementation.
> So it limits API flexibility to some degree.

Ping ddorwin@
David, could you take a look at this and let me know what you think about this?
I think this CL is already better than what we have now, so I'd like to land it
in Chromium if there's no objections.
Also while working on this CL I realized that one more problem that I have not
completely solved here is the problem of mapping which codecs are supported in a
given container. Currently it's being handled through the old code path that
uses the MimeUtil::format_codec_mappings table. But ideally we'd like to make
even that extensible and I'm now trying to figure out a good way to achieve
that. But that change could be a separate CL.

[Ryan Sleevi, 2015-04-21 13:23:29]

Drive by - I'll let David do the more thorough review.

I'm definitely nervous about the performance characteristics here - this just
seems to be substantially worse all around. I don't know how tight of a loop
this is processed in, but surely it can't be good to do it like this, is it? It
also duplicates constant strings a ton, and that's generally an anti-pattern as
well [yes, we have GCC / MSVC doing identical string folding, but better to
declaratively establish them as true constants with kNaming scheme]

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:82: typedef base::Callback<bool(const std::string&)>
IsCodecSupportedCB;
This seems like a silly typedef duplication (you could have used the original
net:: typedef, but personally I think getting rid of both typedefs is more
idiomatic).

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:83: void SetIsCodecSupportedCB(IsCodecSupportedCB
is_codec_supported_cb);
Same remarks re-const-ref.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:509: }
net doesn't use braces for single-line ifs; also line 495-497

(Even if this wasn't a broader //net idiom, the surrounding code - such as
511-513 in the new, or 610-612 / 618-619 of the old should have dictated
consistency w/in a file)

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:784: std::string default_codec = "";
This is the inefficient way to instantiate empty strings

Just use std::string default_codec

In general, prefer "std::string()" over "" whenever you need an empty string
(e.g. for an API call), to avoid the "const char*" conversion. By applying this
rule to all places you'd use "" for std::string, you'd end up with

std::string default_codec = std::string();

which would have made the pattern more obvious.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:899: }
braces

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:916: codec == "vp9" || codec == "vp9.0")
Here you've got multi-line diffs where you should use braces

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:928: codec == "vp9" || codec == "vp9.0")
ditto + braces

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:935: return true;
+braces and +whitespace

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.h#n...
net/base/mime_util.h:116: NET_EXPORT void
SetIsCodecSupportedCB(IsCodecSupportedCB is_codec_supported_cb);
STYLE: Callbacks should *always* be passed as const-ref

DESIGN: Is the typedef actually necessary? It seems cleaner (e.g. avoiding
namespace polution) to avoid it.

[servolk, 2015-04-21 22:09:02]

Re duplicating strings - we can either define constants for codec ids for which
it's possible (e.g. vp8/vp9/vorbis/theora/etc. but for other cases like H264
avc1.*/avc3.* and various MP4 audio formats mp4a.* there's no single codec id
string).
Also, I guess if it's a big concern, perhaps we could merge
IsCodecSupported/IsCodecProprietary/IsCodecAmbiguous into a single function that
would return a combination of 3 flags. That would also reduce code duplication
and string duplication across different functions.
Re performance - I've addressed that at the end of comment #5 above. I don't
think we should be too concerned with perf in this case.
Btw, also keep in mind that we were actually using strings up until ~8 month ago
(see https://codereview.chromium.org/422573005) and the reason we switched to
enums wasn't performance, it's never mentioned in that CL. We just needed to
make sure that canPlayType returns "maybe" for ambiguous codec ids and
"probably" for codecs that are known to be supported. One way to achieve that
was to map codec id strings to enum values and then do the checks with enum
values. But as I've demonstrated in this CL we can actually achieve the same
effect by using string patterns, like 'avc1.*', similar to how it's done in
media/filters/stream_parser_factory.cc for MSE codecs. And using strings
actually makes our code more compact (in terms of lines of code), so I think
it's the right thing to do.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:82: typedef base::Callback<bool(const std::string&)>
IsCodecSupportedCB;
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> This seems like a silly typedef duplication (you could have used the original
> net:: typedef, but personally I think getting rid of both typedefs is more
> idiomatic).

This typedef is redundant indeed, so I've removed it. But regarding completely
getting rid of typedefs, I'm not sure - is there an official guidance for this?
When I search for "typedef base::Callback" in Chromium code I'm getting a lot of
results, even for fairly trivial function signatures (e.g.
net::CompletionCallback; NewKeyCB, DecoderInitCB, DecodeCB, ReadCB in media,
etc). I have no preference here, but I think in general we tend to use typedef
even for trivial callbacks, rather than using base::Callback directly, at least
in media/ But I can get rid of the typedef completely if you insist.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:83: void SetIsCodecSupportedCB(IsCodecSupportedCB
is_codec_supported_cb);
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> Same remarks re-const-ref.

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:509: }
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> net doesn't use braces for single-line ifs; also line 495-497
> 
> (Even if this wasn't a broader //net idiom, the surrounding code - such as
> 511-513 in the new, or 610-612 / 618-619 of the old should have dictated
> consistency w/in a file)

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:784: std::string default_codec = "";
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> This is the inefficient way to instantiate empty strings
> 
> Just use std::string default_codec
> 
> In general, prefer "std::string()" over "" whenever you need an empty string
> (e.g. for an API call), to avoid the "const char*" conversion. By applying
this
> rule to all places you'd use "" for std::string, you'd end up with
> 
> std::string default_codec = std::string();
> 
> which would have made the pattern more obvious.

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:899: }
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> braces

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:916: codec == "vp9" || codec == "vp9.0")
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> Here you've got multi-line diffs where you should use braces

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:928: codec == "vp9" || codec == "vp9.0")
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> ditto + braces

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:935: return true;
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> +braces and +whitespace

Done.

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.h#n...
net/base/mime_util.h:116: NET_EXPORT void
SetIsCodecSupportedCB(IsCodecSupportedCB is_codec_supported_cb);
On 2015/04/21 13:23:29, Ryan Sleevi wrote:
> STYLE: Callbacks should *always* be passed as const-ref
> 
> DESIGN: Is the typedef actually necessary? It seems cleaner (e.g. avoiding
> namespace polution) to avoid it.

Changed to const-ref. Re typedef - I've posted my reasoning in an earlier
comment, let me know which you prefer.

[Ryan Sleevi, 2015-04-22 12:59:57]

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1054943004/diff/240001/net/base/mime_util.cc#...
net/base/mime_util.cc:82: typedef base::Callback<bool(const std::string&)>
IsCodecSupportedCB;
On 2015/04/21 22:09:02, servolk wrote:
> This typedef is redundant indeed, so I've removed it. But regarding completely
> getting rid of typedefs, I'm not sure - is there an official guidance for
this?

We intentionally keep the style guide light - we used to have a lot of 'official
guidance' that no one read or that was more one person's opinion, and it was
trimmed down a lot.

Here's the unofficial guidance - think about the contract and layering of what
you're doing.

For example, if *you*, as a *consumer*, need to store a callback, or use a
callback with the same signature repeatedly (e.g. net::CompletionCallback), then
it may make sense to typedef. If you're just *accepting* a callback, it is
probably better not to.

Why? Typedefs hide what's going on, but save on typing. If you're using a
callback often (net::CompletionCallback), then you can build pattern recognition
skills (it's used often, it's always the same). If a callback is a one-off (as
it is in the header file), then it actually hinders readability by adding a
layer of abstraction between what you want and what you get.

This instance is sort of on the line about "should we shouldn't we" use a
typedef. The argument for is that we end up storing the callback as a member
(line 136); the argument against is that we only use this once (line 83), and so
it hides what's going on.

There's also the general question of whether or not a callback or an interface
is desired. Callbacks can be quite useful, due to type erasure, such that you
can use base::Bind() with any-ol-method. However, they have the downside of
implicitly introducing lifetime management. For example, if you base::Bind()'d a
refcounted class, that refcount would never drop down, because you kept the
callback here in a global. That's "surprising" and, arguably, "bad". Interfaces
make this explicit, because you can explicitly state whether or not ownership of
the interface is transferred (e.g. via scoped_ptr<>), and then make it the
caller's responsibility to keep a refcounted object alive (which would highlight
the code smell) or to just create a throw-away implementation of the interface.

Having said all of that, it wasn't something I thought about at first - because
this is the subtlety of callbacks. Because we're keeping this callback alive
long-term (indefinitely), I think it'd actually be better to declare a proper
interface that can handle the question of "IsXSupported" (or whatever other
questions you want to ask), and make that be passed explicitly (via
scoped_ptr<>), so that it's clear you're giving up ownership to the system to
use as it decides.

Not only does this make lifetime management more explicit, but it also gives you
greater flexibility to consider how the design is. For example, as an interface,
maybe you don't want "IsCodecSupported", but you might want to ask smaller
questions that collectively form an "is supported". Or you might want to use the
enum (and moving it to part of this Delegate interface), which lets you have
your cake and eat it too.

But hopefully this (very long) explanation provides some of the design context
that goes in to choosing whether to typedef or not, whether to callback or not,
etc

> When I search for "typedef base::Callback" in Chromium code I'm getting a lot
of
> results, even for fairly trivial function signatures (e.g.
> net::CompletionCallback; NewKeyCB, DecoderInitCB, DecodeCB, ReadCB in media,
> etc). I have no preference here, but I think in general we tend to use typedef
> even for trivial callbacks, rather than using base::Callback directly, at
least
> in media/ But I can get rid of the typedef completely if you insist.

//media does this heavily, I know, and in my experience it does so in a way that
hurts readability. But that's //media code, and we don't have a style guide
policy on it, and I don't feel strongly enough to push on them, so yeah.

[ddorwin, 2015-04-30 01:15:03]

Not LGTM. I only skimmed it, but it seems clear that this is less maintainable.

Examples:
* One occurrence each of "vp9" and "vp9.0" is replaced with three checks for
both of them.
* Simple easily-readable switch statements are replaced with "if (foo == "bar"
|| foo == "baz" || ...)

[lcwu1, 2015-05-20 02:33:37]

On 2015/04/30 01:15:03, ddorwin wrote:
> Not LGTM. I only skimmed it, but it seems clear that this is less
maintainable.
> 
> Examples:
> * One occurrence each of "vp9" and "vp9.0" is replaced with three checks for
> both of them.
> * Simple easily-readable switch statements are replaced with "if (foo == "bar"
> || foo == "baz" || ...)


Hi David,

This issue is really one of the few remaining items that prevent us from being
able to work directly on the upstream codebase, so we would really like to find
a working solution with you guys. What we really need/care is a mechanism/hook
for a content embedder to override decisions about which codecs/mime types are
supported. I think the concern that you (and other folks) raised about the use
of strings (instead of enums) is valid and we don't have to change it. What do
you think about adding the hooks for content embedders to provide codecs/mime
types checks? If you think it sounds OK, we can clean up the code and upload
another patch to do just that. If not, do you have any suggestions/ideas of what
we can do here? Thanks.

[ddorwin, 2015-05-22 03:45:33]

Some notes I had made.

https://codereview.chromium.org/1054943004/diff/300001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1054943004/diff/300001/net/base/mime_util.cc#...
net/base/mime_util.cc:370: {"audio/mp4", kMP4AudioCodecsExpression},
The explicit list is replaced with a wild card. Presumably, the wild card
succeeds then you check whether it is really a valid codec.

A more direct solution might be to specify functions to call for a given MIME
type. In this case, IsValidMp4AudioCodec. Below, you would have
IsValidMp4VideoCodec, which would check video codecs then call
IsValidMp4AudioCodec.

That abstracts any implementation changes from this struct, though it does add
more functions. Given the clarity and relative simplicity of the functions (plus
the opportunity to avoid parsing these strings), I think it's worth it.

https://codereview.chromium.org/1054943004/diff/300001/net/base/mime_util.cc#...
net/base/mime_util.cc:779: #if defined(ENABLE_MPEG2TS_STREAM_PARSER)
These were added in PS12. If this is new functionality, it should be a separate
CL (along with tests and a BUG). This CL should only be refactoring existing
behavior.

[ddorwin, 2015-05-22 03:52:03]

On 2015/05/20 02:33:37, lcwu_OOO_05.21-06.06 wrote:
> On 2015/04/30 01:15:03, ddorwin wrote:
> > Not LGTM. I only skimmed it, but it seems clear that this is less
> maintainable.
> > 
> > Examples:
> > * One occurrence each of "vp9" and "vp9.0" is replaced with three checks for
> > both of them.
> > * Simple easily-readable switch statements are replaced with "if (foo ==
"bar"
> > || foo == "baz" || ...)
> 
> 
> Hi David,
> 
> This issue is really one of the few remaining items that prevent us from being
> able to work directly on the upstream codebase, so we would really like to
find
> a working solution with you guys. What we really need/care is a mechanism/hook
> for a content embedder to override decisions about which codecs/mime types are
> supported. I think the concern that you (and other folks) raised about the use
> of strings (instead of enums) is valid and we don't have to change it. What do
> you think about adding the hooks for content embedders to provide codecs/mime
> types checks? If you think it sounds OK, we can clean up the code and upload
> another patch to do just that. If not, do you have any suggestions/ideas of
what
> we can do here? Thanks.

I added some thoughts in the doc. I think we should keep as much as possible in
the common code (here). That includes string parsing, interpretation of levels,
etc. Then we can ask individual platforms if they support the parsed result. I'm
not sure we want platforms to be supporting additional strings that the common
code does not know about. That could lead to inconsistent implementations of two
platforms supporting the same thing.

One idea might be to parse the codec string into an object that contains the
existing (or modified) codecs enum along with profile, level, etc. members. This
would allow easy rejection (i.e. THEORA is not supported) as well as more
detailed checks (i.e. high profile is not supported or level 23 is supported.)
Splitting the profiles and levels out into enums migbht actually make the code
more readable since "level 10" can be "0a".

[servolk, 2015-05-22 20:50:41]

On 2015/05/22 03:52:03, ddorwin wrote:
> On 2015/05/20 02:33:37, lcwu_OOO_05.21-06.06 wrote:
> > On 2015/04/30 01:15:03, ddorwin wrote:
> > > Not LGTM. I only skimmed it, but it seems clear that this is less
> > maintainable.
> > > 
> > > Examples:
> > > * One occurrence each of "vp9" and "vp9.0" is replaced with three checks
for
> > > both of them.
> > > * Simple easily-readable switch statements are replaced with "if (foo ==
> "bar"
> > > || foo == "baz" || ...)
> > 
> > 
> > Hi David,
> > 
> > This issue is really one of the few remaining items that prevent us from
being
> > able to work directly on the upstream codebase, so we would really like to
> find
> > a working solution with you guys. What we really need/care is a
mechanism/hook
> > for a content embedder to override decisions about which codecs/mime types
are
> > supported. I think the concern that you (and other folks) raised about the
use
> > of strings (instead of enums) is valid and we don't have to change it. What
do
> > you think about adding the hooks for content embedders to provide
codecs/mime
> > types checks? If you think it sounds OK, we can clean up the code and upload
> > another patch to do just that. If not, do you have any suggestions/ideas of
> what
> > we can do here? Thanks.
> 
> I added some thoughts in the doc. I think we should keep as much as possible
in
> the common code (here). That includes string parsing, interpretation of
levels,
> etc. Then we can ask individual platforms if they support the parsed result.
I'm
> not sure we want platforms to be supporting additional strings that the common
> code does not know about. That could lead to inconsistent implementations of
two
> platforms supporting the same thing.
> 
> One idea might be to parse the codec string into an object that contains the
> existing (or modified) codecs enum along with profile, level, etc. members.
This
> would allow easy rejection (i.e. THEORA is not supported) as well as more
> detailed checks (i.e. high profile is not supported or level 23 is supported.)
> Splitting the profiles and levels out into enums migbht actually make the code
> more readable since "level 10" can be "0a".

Hi David,
Yes, our goal is to keep as much code as possible in common between Chromium and
cast_shell.
When I created this CL ~month ago things were slightly different and this was
IMHO the best way to do things. But one important change that happened since
then is that we finally moved all media mime type and codec checks from
net/base/ into media/base/. This is important, because it means that now we can
get rid of the separate media::MimeUtil::Codec enum, and just use
media::AudioCodec/media::VideoCodec enums and/or
media::AudioDecoderConfig/media::VideoDecoderConfig instead (this was impossible
before, because net/ is not allowed to depend on media/).
So how about we reorganize codec checks slightly like this:
1. Introduce a MimeUtil::ParseAudioCodecId/ParseVideoCodecId methods, that would
take a codec id as a string encoded in mime type (e.g. "avc1.640028" or
"mp4a.40.2") and would return
media::AudioDecoderConfig/media::VideoDecoderConfig object (invalid config
object if parsing failed).
2. If codec id string couldn't be parsed, we obviously don't know how to support
this codec. But if it succeeded, then we do further checks by passing around the
DecoderConfig object, instead of the codec id string. I.e.
IsCodecSupportedOnAndroid and other platform-specific methods would now become
IsAudioDecoderConfigSupportedOnAndroid/IsVideoDecoderConfigSupportedOnAndroid
and they would take DecoderConfig as an input parameter, instead of enum value.
Then in the implementations of those platform-specific methods we could still do
a switch on DecoderConfig.codec() value, but we would also have all the
information originally present in the codec id string (like profile, audio
channel count, video resolution, etc). Since the DecoderConfig object is by
design capable of expressing all this additional information in order to convey
it to deeper layers of media pipeline.
3. And then we could still introduce a callback mechanism similar to one
implemented in this CL, that would allow content embedders to override choices
of supported codecs, but it would also give them the DecoderConfig objects,
instead of string codec ids.
What do you think about this proposal?

[servolk, 2015-05-29 02:07:01]

On 2015/05/22 20:50:41, servolk wrote:
> On 2015/05/22 03:52:03, ddorwin wrote:
> > On 2015/05/20 02:33:37, lcwu_OOO_05.21-06.06 wrote:
> > > On 2015/04/30 01:15:03, ddorwin wrote:
> > > > Not LGTM. I only skimmed it, but it seems clear that this is less
> > > maintainable.
> > > > 
> > > > Examples:
> > > > * One occurrence each of "vp9" and "vp9.0" is replaced with three checks
> for
> > > > both of them.
> > > > * Simple easily-readable switch statements are replaced with "if (foo ==
> > "bar"
> > > > || foo == "baz" || ...)
> > > 
> > > 
> > > Hi David,
> > > 
> > > This issue is really one of the few remaining items that prevent us from
> being
> > > able to work directly on the upstream codebase, so we would really like to
> > find
> > > a working solution with you guys. What we really need/care is a
> mechanism/hook
> > > for a content embedder to override decisions about which codecs/mime types
> are
> > > supported. I think the concern that you (and other folks) raised about the
> use
> > > of strings (instead of enums) is valid and we don't have to change it.
What
> do
> > > you think about adding the hooks for content embedders to provide
> codecs/mime
> > > types checks? If you think it sounds OK, we can clean up the code and
upload
> > > another patch to do just that. If not, do you have any suggestions/ideas
of
> > what
> > > we can do here? Thanks.
> > 
> > I added some thoughts in the doc. I think we should keep as much as possible
> in
> > the common code (here). That includes string parsing, interpretation of
> levels,
> > etc. Then we can ask individual platforms if they support the parsed result.
> I'm
> > not sure we want platforms to be supporting additional strings that the
common
> > code does not know about. That could lead to inconsistent implementations of
> two
> > platforms supporting the same thing.
> > 
> > One idea might be to parse the codec string into an object that contains the
> > existing (or modified) codecs enum along with profile, level, etc. members.
> This
> > would allow easy rejection (i.e. THEORA is not supported) as well as more
> > detailed checks (i.e. high profile is not supported or level 23 is
supported.)
> > Splitting the profiles and levels out into enums migbht actually make the
code
> > more readable since "level 10" can be "0a".
> 
> Hi David,
> Yes, our goal is to keep as much code as possible in common between Chromium
and
> cast_shell.
> When I created this CL ~month ago things were slightly different and this was
> IMHO the best way to do things. But one important change that happened since
> then is that we finally moved all media mime type and codec checks from
> net/base/ into media/base/. This is important, because it means that now we
can
> get rid of the separate media::MimeUtil::Codec enum, and just use
> media::AudioCodec/media::VideoCodec enums and/or
> media::AudioDecoderConfig/media::VideoDecoderConfig instead (this was
impossible
> before, because net/ is not allowed to depend on media/).
> So how about we reorganize codec checks slightly like this:
> 1. Introduce a MimeUtil::ParseAudioCodecId/ParseVideoCodecId methods, that
would
> take a codec id as a string encoded in mime type (e.g. "avc1.640028" or
> "mp4a.40.2") and would return
> media::AudioDecoderConfig/media::VideoDecoderConfig object (invalid config
> object if parsing failed).
> 2. If codec id string couldn't be parsed, we obviously don't know how to
support
> this codec. But if it succeeded, then we do further checks by passing around
the
> DecoderConfig object, instead of the codec id string. I.e.
> IsCodecSupportedOnAndroid and other platform-specific methods would now become
> IsAudioDecoderConfigSupportedOnAndroid/IsVideoDecoderConfigSupportedOnAndroid
> and they would take DecoderConfig as an input parameter, instead of enum
value.
> Then in the implementations of those platform-specific methods we could still
do
> a switch on DecoderConfig.codec() value, but we would also have all the
> information originally present in the codec id string (like profile, audio
> channel count, video resolution, etc). Since the DecoderConfig object is by
> design capable of expressing all this additional information in order to
convey
> it to deeper layers of media pipeline.
> 3. And then we could still introduce a callback mechanism similar to one
> implemented in this CL, that would allow content embedders to override choices
> of supported codecs, but it would also give them the DecoderConfig objects,
> instead of string codec ids.
> What do you think about this proposal?

ping

[ddorwin, 2015-05-29 02:47:41]

On 2015/05/22 20:50:41, servolk wrote:
> Hi David,
> Yes, our goal is to keep as much code as possible in common between Chromium
and
> cast_shell.
> When I created this CL ~month ago things were slightly different and this was
> IMHO the best way to do things. But one important change that happened since
> then is that we finally moved all media mime type and codec checks from
> net/base/ into media/base/. This is important, because it means that now we
can
> get rid of the separate media::MimeUtil::Codec enum, and just use
> media::AudioCodec/media::VideoCodec enums and/or
> media::AudioDecoderConfig/media::VideoDecoderConfig instead (this was
impossible
> before, because net/ is not allowed to depend on media/).
> So how about we reorganize codec checks slightly like this:
> 1. Introduce a MimeUtil::ParseAudioCodecId/ParseVideoCodecId methods, that
would
> take a codec id as a string encoded in mime type (e.g. "avc1.640028" or
> "mp4a.40.2") and would return
> media::AudioDecoderConfig/media::VideoDecoderConfig object (invalid config
> object if parsing failed).

Although some of its members make sense, {Audio|Video}DecoderConfig is overkill.
The name also indicates that it is not appropriate for just codecs.
I think we should work on a separate object, but perhaps we could reuse the new
object as a member of {Audio|Video}DecoderConfig in a future CL.

Having a single codec enum type might be appealing, but
a) The mime_util interfaces would need to support two types (maybe there are
benefits to this).
b) AudioCodec appears to have a lot of differentiation that we don't deal with
in mime_util.
I don't know the reason for (b), but unless they should be the same, we
shouldn't try to make them the same.


> 2. If codec id string couldn't be parsed, we obviously don't know how to
support
> this codec. But if it succeeded, then we do further checks by passing around
the
> DecoderConfig object, instead of the codec id string. I.e.
> IsCodecSupportedOnAndroid and other platform-specific methods would now become
> IsAudioDecoderConfigSupportedOnAndroid/IsVideoDecoderConfigSupportedOnAndroid
> and they would take DecoderConfig as an input parameter, instead of enum
value.
> Then in the implementations of those platform-specific methods we could still
do
> a switch on DecoderConfig.codec() value, but we would also have all the
> information originally present in the codec id string (like profile, audio
> channel count, video resolution, etc). Since the DecoderConfig object is by

At the moment, this code is only used for MIME types and codecs. Audio channel
count and video resolution are not applicable. That could change in the future
if there is a standard for richer feature detection.

> design capable of expressing all this additional information in order to
convey
> it to deeper layers of media pipeline.

Perhaps in the future, but for now, that is inconsistent with both sides of such
an API - there is no way to get the input and platforms don't support obtaining
that information.

> 3. And then we could still introduce a callback mechanism similar to one
> implemented in this CL, that would allow content embedders to override choices
> of supported codecs, but it would also give them the DecoderConfig objects,
> instead of string codec ids.

Ideally, we'd have an interface we could ask rather than relying on a callback.
I don't know if there's an object. Now that this is in media/,
media/base/media_client.h probably makes sense.

> What do you think about this proposal?

[Ryan Sleevi, 2015-07-15 22:50:17]

Is this a dead CL? Should it be closed?

(Trying to clear out my queue)

[servolk, 2015-07-15 23:12:28]

On 2015/07/15 22:50:17, Ryan Sleevi (slow through 7-15 wrote:
> Is this a dead CL? Should it be closed?
> 
> (Trying to clear out my queue)

Yes, this CL is dead in its current form, because media mime type checks are no
longer in net/ anyway. So I'll close it for now.
I'll create a separate CL later to provide media codec checks extensibility
hooks, but that's going to be in media/ and we should be able to sort that out
with media/ folks. Thanks!