Motown: Change media type (stream type) representation

Motown: Change media type (stream type) representation
The old representation had a scheme enum value that combined both medium
(audio/video/etc) and encoding. Additional information was provided via
scheme-dependent 'details'. The new representation has a medium enum
value and an encoding string. Additional information is provided via
medium-dependent 'details' and opaque encoding parameters. This CL also
abandons base64 encoding in favor of array<uint8> for opaque values.
There is also no attempt to represent multiplexed types, and a lot of
wildcard values have been eliminated.

The CL is large but shallow. There are two media (stream) type
representations in the code (mojo and framework), conversions between
the two and between framework and ffmpeg representations, plus some
formatting code (for two representations) used for debugging.

R=kulakowski@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/2a2f7f99c994f8ffd8d18057ce977c844b51b69c

[dalesat, 2016-04-19 18:36:59]

Description was changed from

==========
Motown: Change media type (stream type) representation
The old representation had a scheme enum value that combined both medium
(audio/video/etc) and encoding. Additional information was provided via
scheme-dependent 'details'. The new representation has a medium enum
value and an encoding string. Additional information is provided via
medium-dependent 'details' and opaque encoding parameters. This CL also
abandons base64 encoding in favor of array<uint8> for opaque values.
There is also no attempt to represent multiplexed types, and a lot of
wildcard values have been eliminated.

The CL is large but shallow. There are two media (stream) type
representations in the code (mojo and framework), conversions between
the two and between framework and ffmpeg representations, plus some
formatting code (for two representations) used for debugging.
==========

to

==========
Motown: Change media type (stream type) representation
The old representation had a scheme enum value that combined both medium
(audio/video/etc) and encoding. Additional information was provided via
scheme-dependent 'details'. The new representation has a medium enum
value and an encoding string. Additional information is provided via
medium-dependent 'details' and opaque encoding parameters. This CL also
abandons base64 encoding in favor of array<uint8> for opaque values.
There is also no attempt to represent multiplexed types, and a lot of
wildcard values have been eliminated.

The CL is large but shallow. There are two media (stream) type
representations in the code (mojo and framework), conversions between
the two and between framework and ffmpeg representations, plus some
formatting code (for two representations) used for debugging.
==========

[dalesat, 2016-04-19 18:37:00]

dalesat@chromium.org changed reviewers:
+ jeffbrown@google.com, kulakowski@chromium.org

[dalesat, 2016-04-19 18:38:10]

Please take a look.

This changes the media/stream type representation in preparation for video
support. The video type representation will change considerably in subsequent
CLs.

[kulakowski, 2016-04-19 19:18:23]

Tentative lg2m.

I also think there's a loooot of abort()ing. But I don't think that's your
fault, or that we have any idea how to do better until we have a better idea how
people and the system will consume mojo services. But something to keep an eye
on, which I think you already are, as you have a TODO on one of them.

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:10: const string
kAudioEncodingLpcm = "lpcm";
I feel like I don't know enough about mature media frameworks to know if a
representation of medium and encoding in a string like this will lead you to the
place you want when there are many, many more types. Might be a place for Jeff
to weigh in.

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_demux_impl.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
services/media/factory_service/media_demux_impl.cc:74: const
std::vector<Demux::DemuxStream*>& demux_streams = demux_->streams();
I'm curious why the move away from auto/auto&/auto* etc.

https://codereview.chromium.org/1902183002/diff/1/services/media/framework/st...
File services/media/framework/stream_type.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/framework/st...
services/media/framework/stream_type.cc:20: // These must match the definitions
in media_types.mojom
This comment scares me. Where is the actual comparison between these values and
the mojom ones?

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
File services/media/framework_ffmpeg/av_codec_context.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_codec_context.cc:40: LOG(ERROR) <<
"unsupported av_sample_format " << av_sample_format;
Ditto why the move to this pattern for "not reached".

[dalesat, 2016-04-19 19:47:03]

PTAL

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:10: const string
kAudioEncodingLpcm = "lpcm";
On 2016/04/19 19:18:22, kulakowski wrote:
> I feel like I don't know enough about mature media frameworks to know if a
> representation of medium and encoding in a string like this will lead you to
the
> place you want when there are many, many more types. Might be a place for Jeff
> to weigh in.

Medium isn't encoded as a string. There are four of them, and we'll have to
revise the mojom if we want more.

We want to be able to add encodings when new compression formats are invented,
hence the string. Currently, there's no convention for managing the namespace.
We could consider using a url.

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_demux_impl.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
services/media/factory_service/media_demux_impl.cc:74: const
std::vector<Demux::DemuxStream*>& demux_streams = demux_->streams();
On 2016/04/19 19:18:22, kulakowski wrote:
> I'm curious why the move away from auto/auto&/auto* etc.

I like auto a lot, but previous reviewers have not. These got by somehow. The
guidance I've gotten in past reviews is that auto should only be used when a
type doesn't have a name or when the name is super-obscure. Personally, I prefer
to use auto pretty much whenever the compiler will let me.

https://codereview.chromium.org/1902183002/diff/1/services/media/framework/st...
File services/media/framework/stream_type.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/framework/st...
services/media/framework/stream_type.cc:20: // These must match the definitions
in media_types.mojom
On 2016/04/19 19:18:23, kulakowski wrote:
> This comment scares me. Where is the actual comparison between these values
and
> the mojom ones?

I've added code to verify this as noted in the updated comment.

This list isn't comprehensive (the list of encodings is extensible), so encoding
strings need to be copied when doing conversions between media type (mojom) and
stream type (framework). There's no way a compiler can ensure that all encoding
names are spelled properly.

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
File services/media/framework_ffmpeg/av_codec_context.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_codec_context.cc:40: LOG(ERROR) <<
"unsupported av_sample_format " << av_sample_format;
On 2016/04/19 19:18:23, kulakowski wrote:
> Ditto why the move to this pattern for "not reached".

Preferring LOG(ERROR) over NOTREACHED is feedback from previous CLs. I can't
really speak to why one is better than the other.

I prefer to abort in production code rather than wander off into the weeds with
a bogus value in hand...especially now that there's no representation for
'unknown'. Ideally, we would have some way to abort the service implementation
without killing the rest of the process.

[kulakowski, 2016-04-20 17:42:37]

lgtm

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1902183002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:10: const string
kAudioEncodingLpcm = "lpcm";
On 2016/04/19 19:47:03, dalesat wrote:
> On 2016/04/19 19:18:22, kulakowski wrote:
> > I feel like I don't know enough about mature media frameworks to know if a
> > representation of medium and encoding in a string like this will lead you to
> the
> > place you want when there are many, many more types. Might be a place for
Jeff
> > to weigh in.
> 
> Medium isn't encoded as a string. There are four of them, and we'll have to
> revise the mojom if we want more.
> 
> We want to be able to add encodings when new compression formats are invented,
> hence the string. Currently, there's no convention for managing the namespace.
> We could consider using a url.

Sure.

I suppose I was considering Audio and Video to be types of media, which do occur
in the identifiers, not the string values.

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_demux_impl.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/factory_serv...
services/media/factory_service/media_demux_impl.cc:74: const
std::vector<Demux::DemuxStream*>& demux_streams = demux_->streams();
On 2016/04/19 19:47:03, dalesat wrote:
> On 2016/04/19 19:18:22, kulakowski wrote:
> > I'm curious why the move away from auto/auto&/auto* etc.
> 
> I like auto a lot, but previous reviewers have not. These got by somehow. The
> guidance I've gotten in past reviews is that auto should only be used when a
> type doesn't have a name or when the name is super-obscure. Personally, I
prefer
> to use auto pretty much whenever the compiler will let me.

Makes sense.

I would prefer to use it where it is clear and robust to nonlocal changes.

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
File services/media/framework_ffmpeg/av_codec_context.cc (right):

https://codereview.chromium.org/1902183002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_codec_context.cc:40: LOG(ERROR) <<
"unsupported av_sample_format " << av_sample_format;
On 2016/04/19 19:47:03, dalesat wrote:
> On 2016/04/19 19:18:23, kulakowski wrote:
> > Ditto why the move to this pattern for "not reached".
> 
> Preferring LOG(ERROR) over NOTREACHED is feedback from previous CLs. I can't
> really speak to why one is better than the other.
> 
> I prefer to abort in production code rather than wander off into the weeds
with
> a bogus value in hand...especially now that there's no representation for
> 'unknown'. Ideally, we would have some way to abort the service implementation
> without killing the rest of the process.

Right. One bad agent shouldn't be able to deny service to the rest of the
system.

[dalesat, 2016-04-20 18:07:18]

Description was changed from

==========
Motown: Change media type (stream type) representation
The old representation had a scheme enum value that combined both medium
(audio/video/etc) and encoding. Additional information was provided via
scheme-dependent 'details'. The new representation has a medium enum
value and an encoding string. Additional information is provided via
medium-dependent 'details' and opaque encoding parameters. This CL also
abandons base64 encoding in favor of array<uint8> for opaque values.
There is also no attempt to represent multiplexed types, and a lot of
wildcard values have been eliminated.

The CL is large but shallow. There are two media (stream) type
representations in the code (mojo and framework), conversions between
the two and between framework and ffmpeg representations, plus some
formatting code (for two representations) used for debugging.
==========

to

==========
Motown: Change media type (stream type) representation
The old representation had a scheme enum value that combined both medium
(audio/video/etc) and encoding. Additional information was provided via
scheme-dependent 'details'. The new representation has a medium enum
value and an encoding string. Additional information is provided via
medium-dependent 'details' and opaque encoding parameters. This CL also
abandons base64 encoding in favor of array<uint8> for opaque values.
There is also no attempt to represent multiplexed types, and a lot of
wildcard values have been eliminated.

The CL is large but shallow. There are two media (stream) type
representations in the code (mojo and framework), conversions between
the two and between framework and ffmpeg representations, plus some
formatting code (for two representations) used for debugging.

R=kulakowski@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/2a2f7f99c994f8ffd8d18057ce9...
==========

[dalesat, 2016-04-20 18:07:20]

Committed patchset #3 (id:40001) manually as
2a2f7f99c994f8ffd8d18057ce977c844b51b69c (presubmit successful).