Motown: New wrapper classes for ffmpeg format context and io context

Motown: New wrapper classes for ffmpeg format context and io context
Introduced new wrapper classes for ffmpeg C structures to provide better
lifetime management and provide better homes for some utility functions.
Fixed some issues with return values for readers.

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/18ead1aec2a91a537ac876fcb1a140d3ae2811ab

[dalesat, 2016-03-16 22:01:21]

Description was changed from

==========
Motown: New wrapper classes for ffmpeg format context and io context
Introduced new wrapper classes for ffmpeg C structures to provide better
lifetime management and provide better homes for some utility functions.
Fixed some issues with return values for readers.
==========

to

==========
Motown: New wrapper classes for ffmpeg format context and io context
Introduced new wrapper classes for ffmpeg C structures to provide better
lifetime management and provide better homes for some utility functions.
Fixed some issues with return values for readers.
==========

[dalesat, 2016-03-16 22:01:21]

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

[dalesat, 2016-03-16 22:01:34]

Please take a look. Thanks!

[johngro, 2016-03-21 21:06:08]

small stuff only.

FWIW - there is a pattern here which is repeated 3-or-so times where you have

FFmpegObject

which you expose as a

mojo::media::FfmpegObjectPtr

And have a related

mojo::media::FfmpegObject

which has static methods for creating manipulating FfmpegObjectPtr  instances.

Probably not this change, but if you were interested, you could flip this
relationship around so that mojo::media::FfmpegObject was just a wrapper around
a unique_ptr and had move-only semantics.  This would let you keep your
manipulation methods as methods instead of static methods.  The boilerplate
definitions of the rvalue ref assignment and constructor could be factored away
using a template, or just re-implemented given that there are so few of these.

Just a thought.

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

https://codereview.chromium.org/1814583002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_io_context.cc:27: const int kBufferSize = 32
* 1024;
constexpr

https://codereview.chromium.org/1814583002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_io_context.cc:29: InitFfmpeg();
your other FFmpeg helper classes/structs seem to make the assumption that FFmpeg
has already been initialized by the environment, AvIoContext probably should as
well.

If you have a IsFfmpegInited() function somewhere (or one is easy to write), it
might be a good idea to DCHECK that in all of these cases.

https://codereview.chromium.org/1814583002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_io_context.cc:67: DCHECK(size <=
std::numeric_limits<int64_t>::max());
this DCHECK can never fail, so its probably OK to just drop it.

[dalesat, 2016-03-21 22:22:19]

dalesat@chromium.org changed reviewers:
+ kulakowski@chromium.org

[dalesat, 2016-03-22 00:31:36]

PTAL

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

https://codereview.chromium.org/1814583002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_io_context.cc:29: InitFfmpeg();
On 2016/03/21 21:06:08, johngro wrote:
> your other FFmpeg helper classes/structs seem to make the assumption that
FFmpeg
> has already been initialized by the environment, AvIoContext probably should
as
> well.
> 
> If you have a IsFfmpegInited() function somewhere (or one is easy to write),
it
> might be a good idea to DCHECK that in all of these cases.

Done.

https://codereview.chromium.org/1814583002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_io_context.cc:67: DCHECK(size <=
std::numeric_limits<int64_t>::max());
On 2016/03/21 21:06:08, johngro wrote:
> this DCHECK can never fail, so its probably OK to just drop it.

Done.

[johngro, 2016-03-22 16:41:46]

lgtm

[johngro, 2016-03-22 16:41:48]

lgtm

[dalesat, 2016-03-22 19:17:31]

Description was changed from

==========
Motown: New wrapper classes for ffmpeg format context and io context
Introduced new wrapper classes for ffmpeg C structures to provide better
lifetime management and provide better homes for some utility functions.
Fixed some issues with return values for readers.
==========

to

==========
Motown: New wrapper classes for ffmpeg format context and io context
Introduced new wrapper classes for ffmpeg C structures to provide better
lifetime management and provide better homes for some utility functions.
Fixed some issues with return values for readers.

R=johngro@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/18ead1aec2a91a537ac876fcb1a...
==========

[dalesat, 2016-03-22 19:17:32]

Committed patchset #2 (id:20001) manually as
18ead1aec2a91a537ac876fcb1a140d3ae2811ab (presubmit successful).