Motown: ffmpeg implementations of framework 'parts'

Motown: ffmpeg implementations of framework 'parts'
ffmpeg_video_decoder.* is not yet functional

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/ccde3cc6961adbb482d6aea82ed2f9c07959b9bc

[dalesat, 2016-02-11 01:41:40]

Description was changed from

==========
Motown: ffmpeg implementations of framework 'parts'
ffmpeg_video_decoder.* is not yet functional
==========

to

==========
Motown: ffmpeg implementations of framework 'parts'
ffmpeg_video_decoder.* is not yet functional
==========

[dalesat, 2016-02-11 01:41:40]

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

[dalesat, 2016-02-11 02:07:51]

The trybots are having problems applying this patch. Investigating.

[johngro, 2016-03-01 01:31:39]

Almost done; I'm still finishing up audio_decoder.cc and decoder.cc.  I'll have
them done first thing tomorrow morning.

Top level comment in the meantime:
While I have made a few comments which boil down to things like "this was leaked
here", the majority of my comments are things like "it might be better to do XYZ
this other way".

If you are pushing to get this code checked in ASAP, then for the latter type of
comment...

1) If you agree with me and are willing to handle it this CL, please do so.
2) If you agree with me, but don't want to handle it this CL, please leave a
TODO at least, and write up an issue for extra credit.
3) If you disagree with me, but are willing to debate it at a later time, please
leave a TODO.
4) If you disagree with me and will tolerate no dissent, then give your reasons
and we can go from there.

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
build/config/compiler/BUILD.gn:854: "-Wno-constant-conversion",
Is it possible to stuff this in the build rules for just ffmpeg instead of
globally?

If not, you probably want to run this by jamesr@; disabling a warning like this
globally seems like a pretty big deal.  If we end up doing this, we definitely
need to log a bug (like has been done by some people above).

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:75: presentation_time =
next_presentation_time_;
I don't think that you should do this.  If the generated frame contains no PTS,
interpolation should be left to the renderer.

The exception to this would be immediately after a discontinuity.  Presuming
that the discontinuity is caused by a seek operation (initial startup being just
a special case of seek), then the container should be producing the initial
timestamp.

In the very special case of a container which contains absolutely no timestamps,
the demux code should simply put an arbitrary timestamp on the first payload and
NOPTS on subsequent payloads.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_decoder_base.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:22: return Result::kOk;
Since this class is already abstract, why provide an implementation of
Decoder::Init which does nothing but return OK?  Why not just leave it as
abstract and let the derived class implement it?

Turning it around...  If a derived class implements Decode and CreatePacket, but
not Init, then no one is going to create an AVCodecContex and
output_stream_type() (below) is going to crash.

Alternatively, if you implemented Init here to create a dummy AVCodecContext
whose stream type was STREAM_TYPE_NOTHING or something, you could avoid this,
but leaving Init as abstract seems like a simpler choice.

FWIW - strictly speaking, no data members are supposed to be protected (only
methods).  Perhaps..
1) Init(const StreamType&) should remain abstract
2) Init(unique_ptr<AVCodecContext>) should exist, be non-virtual, and be
protected
3) av_codec_context_ should be made private (with a const unique_ptr<>&
av_codec_context accessor?)

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:30:
avcodec_flush_buffers(av_codec_context_.get());
avcodec_flush_buffers is not safe against being called with NULL.  Suggest 

DCHECK(av_codec_context_);

before making this call.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:49: int input_bytes_used
= Decode(allocator, &frame_decoded);
ssize_t instead of int?

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:60: av_packet_.data +=
input_bytes_used;
DCHECK(av_packet_.size >= input_bytes_used) and runtime limit input_bytes_used
to be no larger than av_packet_.size

Alternatively; if the decoder claims to have used more bytes than the packet
allows, perhaps the decode should be declared to be a failure.

Right now, there is a risk of underflowing av_packet_.size and leaving
av_packet_.data as a bad pointer.  This opens the possibility for the check
below to decide that we are not done with the ES packet yet, and to continue
processing it, but with a bad size and data pointer next time around.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:87: *output =
Packet::CreateEndOfStream(next_presentation_time_);
what is the purpose of putting a PTS on an empty EOS packet?  IOW - why not just
leave it as NOPTS?

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:101:
next_presentation_time_ = packet->presentation_time() + packet->duration();
either presentation_time or duration may be undefined here (ffmpeg uses the
constant AV_NOPTS_VALUE).  If we remove next_presentation_time, we don't need to
worry about this, but if we want to keep it in order to put a PTS on generated
empty EOS frames, then we need to handle these cases (and probably just put
NOPTS into next_presentation_time_)

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_decoder_base.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.h:17: class
FfmpegDecoderBase : public FfmpegDecoder {
This could just derive from Decoder instead of FfmpegDecoder, since
FfmpegDecoder is basically just acting as a namespace for the Create method.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_demux.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:55: ~DemuxPacket() override {
Packet's destructor is protected, DemuxPacket's should be as well.

Also; since Packets are created using Packet::Create* which each return a
PacketPtr (which is a unique_ptr with a custom deleter), the Packet constructor
(as well as the constructor's of it's derived classes) should probably be
protected as well.

All of this said, see comments attached to Release below

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:56: av_free_packet(&av_packet);
what happens if the av_packet was never allocated by ffmpeg?

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:64: bool end_of_stream() const
override { return false; }
if PTS, duration and EOS are core pieces of metadata, consider leaving them down
in the Packet level instead of putting them into a custom FFmpeg DemuxPacket
type.

Also, the NOPTS sentinel value used by FFmpeg is different from one that we have
defined.  We should either align our values, or we need to introduce a place to
perform a translation.  These virtual methods might be a place to do such a
thing, but I think that it would be better to do so at creation time, instead of
every time the value is queried.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:72: AVPacket av_packet;
this should be private and named av_packet_

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:75: void Release() override {
delete this; }
so, Packet instances seem to be managed using a std::unique_ptr.  Release is
getting called from the unique_ptr's deleter, which (in this implementation)
calls delete, which then uses the ffmpeg free function to free av_packet.

So, why do we need Release?  IOW - why can't we...

1) Expose only unique_ptrs by using the Create patterns and burying the
constructor.
2) Make the destructor public (so unique_ptr can access it)
3) Implement all custom destruction behavior in the destructor (as is done here)

Right now, my concern is that something could (in theory) call Release while the
unique_ptr is still in scope leaving us with a dead pointer referenced by the
PacketPtr.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:117: static const uint64_t
kNanosecondsPerMicrosecond = 1000;
constexpr

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:125: LOG(ERROR) <<
"CreateAvioContext failed (allocation failure)";
this leaks format_context

I made a similar comment to what follows in the Demux code, but...

Consider extracting management of FFmpeg's unmanged pointer types into some
combination of utility class/namespace.  If you pull stuff like the
AVFormatContextDeleter out of here an put it into the utility region of code,
then you can restrict things to just manipulating the unique pointers, and these
leak hazards should go away.  As part of this, you would do things like bury all
calls FFmpeg's alloc and free functions (the would only ever be invoked from the
utility layer), and this code would become more like

AVFormatContextUPtr format_context = utils::FFmpegAllocAVFormatContext();

// do stuff which might fail and automatically clean up format_context

format_context_ = std::move(format_context);  // explicit ownership transfer


The same potentially hazardous pattern exists here for io_context_, although the
same issue does not as the io_context instance is stored in an io_context
unique_ptr from the get-go.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:132: int r =
avformat_open_input(&format_context, nullptr, nullptr, nullptr);
These two operations (open_input and find_stream_info) are blocking operations
in the land of FFmpeg.  Additionally, they have a relatively high probability of
both taking a significant amount of time and eventually failing.

While I understand that shortcuts were taken in the name of producing a demo on
time, these hazards should really be documented with a TODO in the code, and a
bug filed as soon as the code is checked in.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:190:
av_init_packet(&demux_packet->av_packet);
FfmpegDemuxImpl::DemuxPacket owns the call to av_free_packet, it should own the
call to av_init_packet as well.

IOW - no matter what happens, it should be impossible to create an instance of a
FfmpegDemuxImpl::DemuxPacket with an uninitialized av_packet.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:192:
demux_packet->av_packet.size = 0;
access to av_packet should exposed using an accessor (AVPacket& av_packet() {
return av_packet_; } instead of by making the variable public.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:204:
demux_packet->presentation_time() + demux_packet->duration();
either PTS or duration may be AV_NOPTS_VALUE and need to be handled
appropriately.  See earlier comment.

In general, it would be nice to just eliminate the need for
next_presentation_time_.  Right now, it seems to exist only for the purpose of
putting a timestamp on EOS payloads.

Finally, the demux seems to be putting timestamps on generated EOS payloads, it
does not seem like there should be a need to either generate or timestamp EOS
payloads at the decoder level.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:239: AVFormatContext*
format_context,
Ownership of format_context is not being transferred here, nor is it being
modified, nor may it be NULL.

This should probably be a "const AVFormatContext&" instead of a raw pointer.  If
null-ness is permitted in this system, then it should be a "const
std::unique_ptr<AVFormatContext>&" and a either DCHECK should be run before
accessing the pointer, or proper runtime behavior should be implemented.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_init.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_init.cc:15: static bool initialized_ =
false;
While idempotent in a purely single threaded world, this is not thread safe.

I think that it is perfectly reasonable to say that it needs to be neither
idempotent nor thread-safe as this code should probably be called only once very
early in an application's main.  By this argument, you could remove the
initialized flag entirely and just call av_register_all().

If you want some protection against an accidental double call, you could use
std::atomic to do a compare and exchange, and then DCHECK that the method was
not already called.  This enforces the call-once requirement, and is thread-safe
for practically no cost (even though it should not need to, since the rule is
probably call-once only).

If it should be possible from an API perspective to call this multiple times,
then this becomes more complicated to make thread safe.  One would need to both
avoid the double init, but also use a synchronization primitive of some form to
ensure that the post-condition (FFmpeg has been fully initialized) has been met
before the second call to init exits.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_io.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_io.h:17: AVIOContext*
CreateAvioContext(Reader* reader);
see earlier comment about raw FFmpeg pointers.  Consider wrapping all of this
stuff (creation/destruction) up in a unique_ptr type which is defined here and
disallowing anything but this code from ever accessing the FFmpeg create/destroy
functions.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.cc:42: uint8_t*
ExtraDataFromBytes(const Bytes& bytes, int* byte_count_out) {
This function seems to only be used to populate the extradata and extradata_size
fields of an AVCodecContext structure.  Since these fields are owned by the
AVCodecContext (and therefor, the FFmpeg implementation), perhaps it would be
better to have this function take a pointer to an AVCodecContext which it
populates, and return either void or some success/failure indication (either a
bool, or a Result or something).

This way, we never need to expose a naked pointer to anything else, and can
centralize things like error checking.  For example, decisions to do something
like free an already existing extradata, or to DCHECK that there isn't one,
could be implemented here.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.cc:135: #undef
PixelFormat
oi, global #defs from 3rd party code.

Can we #undef this at the top, just after the #includes?

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:20:
std::unique_ptr<StreamType> StreamTypeFromAVCodecContext(AVCodecContext* from);
can we make AVCodecContext be a const unique_ptr<> & here (and a unique_ptr<> in
the return from AVCodecContextFromStreamType below?)

Because of the custom deleter, it might be best to hoist the deleter functor out
of the FfmpegDecoderBase class and place it at a utility level.  Generally, I
think it would be best to put its name in an internal {} namespace and
typedef/using the pointer type so that people don't have to write things like
unique_ptr<AVCodecContext, internal::AVCodecContextDeleter>.

I know that Jeff disagrees somewhat with the typedef approach (argument; it
removes clarity about the underlying type of the pointer), but perhaps he would
be OK with it if it was something like

using AVCodecContextUPtr = ...;

or even

using AVCodecContextUniquePtr = ...;

My counter to Jeff's point would be that users of a unique pointer's class
should not know about details like custom deleters.  Also, if in attempting to
avoid the need for knowledge of things like custom deleters, we end up returning
unmanaged pointers for newly created object from factory functions (things like
AVCodecContextFromStreamType below), then we are defeating the purpose of using
the managed pointer classes in the first place.

[jamesr, 2016-03-01 17:56:43]

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
build/config/compiler/BUILD.gn:854: "-Wno-constant-conversion",
On 2016/03/01 at 01:31:38, johngro wrote:
> Is it possible to stuff this in the build rules for just ffmpeg instead of
globally?
> 
> If not, you probably want to run this by jamesr@; disabling a warning like
this globally seems like a pretty big deal.  If we end up doing this, we
definitely need to log a bug (like has been done by some people above).

Don't disable warnings like this globally.  You can add this to the cflags for
the ffmpeg target (or targets) that trigger the warning.

[dalesat, 2016-03-01 18:00:00]

dalesat@chromium.org changed reviewers:
+ smklein@google.com

[johngro, 2016-03-01 18:05:35]

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:18: if
(av_sample_fmt_is_planar(av_codec_context->sample_fmt)) {
DCHECK(av_codec_context_->channels);

There is a non-zero assumption made for this field in the planar path of
Allocate below.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:99: payload_buffer =
av_frame_->data[0];
So, I think that ffmpeg is going to end up freeing your buffer out from under
you in the interleaved case.  The sequence would be..

1) During decode, ffmpeg calls your AllocateBufferForAvFrame hook
2) A single buffer is allocated using self->allocator_.
2.1) av_frame_->data[0] now points to this buffer.
2.2) the call to av_buffer_create also holds a pointer to the buffer as well as
the hook for ReleaseBufferForAvFrame.
3) After decode, framework calls PacketFromAvFrame.
3.1) PacketFromAvFrame calls CreateOutputPacket (here in the code), and you grab
a pointer to the buffer, but do not detach the buffer from the av_frame_
instance.
3.2) Packet::Create is called and passed a pointer to the buffer.
3.3) PacketFromAvFrame calls av_frame_unref.
3.4) av_frame_unref eventually calls the ReleaseBufferHook which frees the
buffer.
4) The packet is sent down the line.  It is holding a dead pointer.  Eventually
(assuming it has not already crashed) it will double-free this pointer.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:133: if (buffer_size <
0) {
<= 0

Also, need to make sure that the rest of the code will produce coherent results
if you shortcut at this point (av_buffer_create is not going to be called if you
bail out now, for example).

That, or make sure the code below will tolerate a 0 buffer size.  Right now,
AllocatePayloadBuffer in the default allocator will not tolerate a request to
allocate 0 bytes.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:185: // We lose the
buffer above before CreatePacket gets called, so we save the
Why do we lose the buffer?  Isn't the pointer to it stored in av_frame->buf[0]? 
It is not an opaque data structure, you should just be able to use the size/data
members of av_frame->buf[0] instead of needing to stash values in the class.

[dalesat, 2016-03-01 20:43:02]

PTAL. I've left the warning change for the moment. I'll work on getting rid of
it.

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1686363002/diff/100001/build/config/compiler/...
build/config/compiler/BUILD.gn:854: "-Wno-constant-conversion",
On 2016/03/01 17:56:43, jamesr wrote:
> On 2016/03/01 at 01:31:38, johngro wrote:
> > Is it possible to stuff this in the build rules for just ffmpeg instead of
> globally?
> > 
> > If not, you probably want to run this by jamesr@; disabling a warning like
> this globally seems like a pretty big deal.  If we end up doing this, we
> definitely need to log a bug (like has been done by some people above).
> 
> Don't disable warnings like this globally.  You can add this to the cflags for
> the ffmpeg target (or targets) that trigger the warning.

FWIW, I'm restoring a flag that was only recently removed. I've made several
attempts to do this more locally to no avail. We may need our own branch of
ffmpeg to achieve this.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:75: presentation_time =
next_presentation_time_;
On 2016/03/01 01:31:38, johngro wrote:
> I don't think that you should do this.  If the generated frame contains no
PTS,
> interpolation should be left to the renderer.
> 
> The exception to this would be immediately after a discontinuity.  Presuming
> that the discontinuity is caused by a seek operation (initial startup being
just
> a special case of seek), then the container should be producing the initial
> timestamp.
> 
> In the very special case of a container which contains absolutely no
timestamps,
> the demux code should simply put an arbitrary timestamp on the first payload
and
> NOPTS on subsequent payloads.

We should have design discussions like this in another forum.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_decoder_base.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:22: return Result::kOk;
On 2016/03/01 01:31:38, johngro wrote:
> Since this class is already abstract, why provide an implementation of
> Decoder::Init which does nothing but return OK?  Why not just leave it as
> abstract and let the derived class implement it?
> 
> Turning it around...  If a derived class implements Decode and CreatePacket,
but
> not Init, then no one is going to create an AVCodecContex and
> output_stream_type() (below) is going to crash.
> 
> Alternatively, if you implemented Init here to create a dummy AVCodecContext
> whose stream type was STREAM_TYPE_NOTHING or something, you could avoid this,
> but leaving Init as abstract seems like a simpler choice.
> 
> FWIW - strictly speaking, no data members are supposed to be protected (only
> methods).  Perhaps..
> 1) Init(const StreamType&) should remain abstract
> 2) Init(unique_ptr<AVCodecContext>) should exist, be non-virtual, and be
> protected
> 3) av_codec_context_ should be made private (with a const unique_ptr<>&
> av_codec_context accessor?)

AVCodecContext is established in the constructor above. Init isn't needed, and
neither subclass implements it. I'd rather remove it from the Decoder
definition.

There should be no protected fields. I got that feedback in a previous CL and
missed it here. Fixing.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:30:
avcodec_flush_buffers(av_codec_context_.get());
On 2016/03/01 01:31:38, johngro wrote:
> avcodec_flush_buffers is not safe against being called with NULL.  Suggest 
> 
> DCHECK(av_codec_context_);
> 
> before making this call.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:49: int input_bytes_used
= Decode(allocator, &frame_decoded);
On 2016/03/01 01:31:38, johngro wrote:
> ssize_t instead of int?

Decode wraps one of two avcodec_decode_* functions, both of which return int.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:60: av_packet_.data +=
input_bytes_used;
On 2016/03/01 01:31:38, johngro wrote:
> DCHECK(av_packet_.size >= input_bytes_used) and runtime limit input_bytes_used
> to be no larger than av_packet_.size
> 
> Alternatively; if the decoder claims to have used more bytes than the packet
> allows, perhaps the decode should be declared to be a failure.
> 
> Right now, there is a risk of underflowing av_packet_.size and leaving
> av_packet_.data as a bad pointer.  This opens the possibility for the check
> below to decide that we are not done with the ES packet yet, and to continue
> processing it, but with a bad size and data pointer next time around.

Added a CHECK

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:87: *output =
Packet::CreateEndOfStream(next_presentation_time_);
On 2016/03/01 01:31:38, johngro wrote:
> what is the purpose of putting a PTS on an empty EOS packet?  IOW - why not
just
> leave it as NOPTS?

Design discussion.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.cc:101:
next_presentation_time_ = packet->presentation_time() + packet->duration();
On 2016/03/01 01:31:38, johngro wrote:
> either presentation_time or duration may be undefined here (ffmpeg uses the
> constant AV_NOPTS_VALUE).  If we remove next_presentation_time, we don't need
to
> worry about this, but if we want to keep it in order to put a PTS on generated
> empty EOS frames, then we need to handle these cases (and probably just put
> NOPTS into next_presentation_time_)

I've moved the next_presentation_time_ into the subclasses, so we won't have a
dependency on the doomed duration field.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_decoder_base.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_decoder_base.h:17: class
FfmpegDecoderBase : public FfmpegDecoder {
On 2016/03/01 01:31:38, johngro wrote:
> This could just derive from Decoder instead of FfmpegDecoder, since
> FfmpegDecoder is basically just acting as a namespace for the Create method.
> 

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_demux.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:55: ~DemuxPacket() override {
On 2016/03/01 01:31:38, johngro wrote:
> Packet's destructor is protected, DemuxPacket's should be as well.
> 
> Also; since Packets are created using Packet::Create* which each return a
> PacketPtr (which is a unique_ptr with a custom deleter), the Packet
constructor
> (as well as the constructor's of it's derived classes) should probably be
> protected as well.
> 
> All of this said, see comments attached to Release below

Made the destructor protected. Packet::Create creates packets with the standard
implementation, not DemuxPacket.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:56: av_free_packet(&av_packet);
On 2016/03/01 01:31:39, johngro wrote:
> what happens if the av_packet was never allocated by ffmpeg?

Not sure what you mean. This packet implementation is specifically for wrapping
av_packet.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:64: bool end_of_stream() const
override { return false; }
On 2016/03/01 01:31:39, johngro wrote:
> if PTS, duration and EOS are core pieces of metadata, consider leaving them
down
> in the Packet level instead of putting them into a custom FFmpeg DemuxPacket
> type.
> 
> Also, the NOPTS sentinel value used by FFmpeg is different from one that we
have
> defined.  We should either align our values, or we need to introduce a place
to
> perform a translation.  These virtual methods might be a place to do such a
> thing, but I think that it would be better to do so at creation time, instead
of
> every time the value is queried.

We already have TODOs to revisit the packet definition, including getting rid of
the custom accessors.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:72: AVPacket av_packet;
On 2016/03/01 01:31:39, johngro wrote:
> this should be private and named av_packet_

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:75: void Release() override {
delete this; }
On 2016/03/01 01:31:39, johngro wrote:
> so, Packet instances seem to be managed using a std::unique_ptr.  Release is
> getting called from the unique_ptr's deleter, which (in this implementation)
> calls delete, which then uses the ffmpeg free function to free av_packet.
> 
> So, why do we need Release?  IOW - why can't we...
> 
> 1) Expose only unique_ptrs by using the Create patterns and burying the
> constructor.
> 2) Make the destructor public (so unique_ptr can access it)
> 3) Implement all custom destruction behavior in the destructor (as is done
here)
> 
> Right now, my concern is that something could (in theory) call Release while
the
> unique_ptr is still in scope leaving us with a dead pointer referenced by the
> PacketPtr.

We already have TODOs to revisit this. I took this approach so we could have a
lookaside of Packet instances.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:117: static const uint64_t
kNanosecondsPerMicrosecond = 1000;
On 2016/03/01 01:31:39, johngro wrote:
> constexpr

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:125: LOG(ERROR) <<
"CreateAvioContext failed (allocation failure)";
On 2016/03/01 01:31:38, johngro wrote:
> this leaks format_context
> 
> I made a similar comment to what follows in the Demux code, but...
> 
> Consider extracting management of FFmpeg's unmanged pointer types into some
> combination of utility class/namespace.  If you pull stuff like the
> AVFormatContextDeleter out of here an put it into the utility region of code,
> then you can restrict things to just manipulating the unique pointers, and
these
> leak hazards should go away.  As part of this, you would do things like bury
all
> calls FFmpeg's alloc and free functions (the would only ever be invoked from
the
> utility layer), and this code would become more like
> 
> AVFormatContextUPtr format_context = utils::FFmpegAllocAVFormatContext();
> 
> // do stuff which might fail and automatically clean up format_context
> 
> format_context_ = std::move(format_context);  // explicit ownership transfer
> 
> 
> The same potentially hazardous pattern exists here for io_context_, although
the
> same issue does not as the io_context instance is stored in an io_context
> unique_ptr from the get-go.

Moved this allocation down to where it belongs. Added a comment proposing the
utility class.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:132: int r =
avformat_open_input(&format_context, nullptr, nullptr, nullptr);
On 2016/03/01 01:31:39, johngro wrote:
> These two operations (open_input and find_stream_info) are blocking operations
> in the land of FFmpeg.  Additionally, they have a relatively high probability
of
> both taking a significant amount of time and eventually failing.
> 
> While I understand that shortcuts were taken in the name of producing a demo
on
> time, these hazards should really be documented with a TODO in the code, and a
> bug filed as soon as the code is checked in.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:132: int r =
avformat_open_input(&format_context, nullptr, nullptr, nullptr);
On 2016/03/01 01:31:39, johngro wrote:
> These two operations (open_input and find_stream_info) are blocking operations
> in the land of FFmpeg.  Additionally, they have a relatively high probability
of
> both taking a significant amount of time and eventually failing.
> 
> While I understand that shortcuts were taken in the name of producing a demo
on
> time, these hazards should really be documented with a TODO in the code, and a
> bug filed as soon as the code is checked in.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:190:
av_init_packet(&demux_packet->av_packet);
On 2016/03/01 01:31:39, johngro wrote:
> FfmpegDemuxImpl::DemuxPacket owns the call to av_free_packet, it should own
the
> call to av_init_packet as well.
> 
> IOW - no matter what happens, it should be impossible to create an instance of
a
> FfmpegDemuxImpl::DemuxPacket with an uninitialized av_packet.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:192:
demux_packet->av_packet.size = 0;
On 2016/03/01 01:31:39, johngro wrote:
> access to av_packet should exposed using an accessor (AVPacket& av_packet() {
> return av_packet_; } instead of by making the variable public.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:204:
demux_packet->presentation_time() + demux_packet->duration();
On 2016/03/01 01:31:39, johngro wrote:
> either PTS or duration may be AV_NOPTS_VALUE and need to be handled
> appropriately.  See earlier comment.
> 
> In general, it would be nice to just eliminate the need for
> next_presentation_time_.  Right now, it seems to exist only for the purpose of
> putting a timestamp on EOS payloads.
> 
> Finally, the demux seems to be putting timestamps on generated EOS payloads,
it
> does not seem like there should be a need to either generate or timestamp EOS
> payloads at the decoder level.

Acknowledged.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:239: AVFormatContext*
format_context,
On 2016/03/01 01:31:39, johngro wrote:
> Ownership of format_context is not being transferred here, nor is it being
> modified, nor may it be NULL.
> 
> This should probably be a "const AVFormatContext&" instead of a raw pointer. 
If
> null-ness is permitted in this system, then it should be a "const
> std::unique_ptr<AVFormatContext>&" and a either DCHECK should be run before
> accessing the pointer, or proper runtime behavior should be implemented.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_init.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_init.cc:15: static bool initialized_ =
false;
On 2016/03/01 01:31:39, johngro wrote:
> While idempotent in a purely single threaded world, this is not thread safe.
> 
> I think that it is perfectly reasonable to say that it needs to be neither
> idempotent nor thread-safe as this code should probably be called only once
very
> early in an application's main.  By this argument, you could remove the
> initialized flag entirely and just call av_register_all().
> 
> If you want some protection against an accidental double call, you could use
> std::atomic to do a compare and exchange, and then DCHECK that the method was
> not already called.  This enforces the call-once requirement, and is
thread-safe
> for practically no cost (even though it should not need to, since the rule is
> probably call-once only).
> 
> If it should be possible from an API perspective to call this multiple times,
> then this becomes more complicated to make thread safe.  One would need to
both
> avoid the double init, but also use a synchronization primitive of some form
to
> ensure that the post-condition (FFmpeg has been fully initialized) has been
met
> before the second call to init exits.

Added a lock.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_io.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_io.h:17: AVIOContext*
CreateAvioContext(Reader* reader);
On 2016/03/01 01:31:39, johngro wrote:
> see earlier comment about raw FFmpeg pointers.  Consider wrapping all of this
> stuff (creation/destruction) up in a unique_ptr type which is defined here and
> disallowing anything but this code from ever accessing the FFmpeg
create/destroy
> functions.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.cc:42: uint8_t*
ExtraDataFromBytes(const Bytes& bytes, int* byte_count_out) {
On 2016/03/01 01:31:39, johngro wrote:
> This function seems to only be used to populate the extradata and
extradata_size
> fields of an AVCodecContext structure.  Since these fields are owned by the
> AVCodecContext (and therefor, the FFmpeg implementation), perhaps it would be
> better to have this function take a pointer to an AVCodecContext which it
> populates, and return either void or some success/failure indication (either a
> bool, or a Result or something).
> 
> This way, we never need to expose a naked pointer to anything else, and can
> centralize things like error checking.  For example, decisions to do something
> like free an already existing extradata, or to DCHECK that there isn't one,
> could be implemented here.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.cc:135: #undef
PixelFormat
On 2016/03/01 01:31:39, johngro wrote:
> oi, global #defs from 3rd party code.
> 
> Can we #undef this at the top, just after the #includes?

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:20:
std::unique_ptr<StreamType> StreamTypeFromAVCodecContext(AVCodecContext* from);
On 2016/03/01 01:31:39, johngro wrote:
> can we make AVCodecContext be a const unique_ptr<> & here (and a unique_ptr<>
in
> the return from AVCodecContextFromStreamType below?)
> 
> Because of the custom deleter, it might be best to hoist the deleter functor
out
> of the FfmpegDecoderBase class and place it at a utility level.  Generally, I
> think it would be best to put its name in an internal {} namespace and
> typedef/using the pointer type so that people don't have to write things like
> unique_ptr<AVCodecContext, internal::AVCodecContextDeleter>.
> 
> I know that Jeff disagrees somewhat with the typedef approach (argument; it
> removes clarity about the underlying type of the pointer), but perhaps he
would
> be OK with it if it was something like
> 
> using AVCodecContextUPtr = ...;
> 
> or even
> 
> using AVCodecContextUniquePtr = ...;
> 
> My counter to Jeff's point would be that users of a unique pointer's class
> should not know about details like custom deleters.  Also, if in attempting to
> avoid the need for knowledge of things like custom deleters, we end up
returning
> unmanaged pointers for newly created object from factory functions (things
like
> AVCodecContextFromStreamType below), then we are defeating the purpose of
using
> the managed pointer classes in the first place.

When a custom deleter is in play, I too prefer to define the pointer type.

Even if we're always managing Foo with a unique_ptr, I prefer passing const Foo&
to passing const std::unique_ptr<Foo>&.

[jamesr, 2016-03-01 20:47:33]

What issues are you having applying the flag more locally?  Keeping warnings
like this enabled is a significant part of maintaining code quality and I'd be
very reluctant to disable something globally when only one small (relative to
the rest of the codebase) has issues with it.

[johngro, 2016-03-01 22:07:27]

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_demux.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:56: av_free_packet(&av_packet);
On 2016/03/01 20:43:02, dalesat wrote:
> On 2016/03/01 01:31:39, johngro wrote:
> > what happens if the av_packet was never allocated by ffmpeg?
> 
> Not sure what you mean. This packet implementation is specifically for
wrapping
> av_packet.

yes, but nothing in this class made certain that av_packet_ had been properly
initialized.  Before, if an instance of the class was created then immediately
destroyed, av_free_packet was going to get called with an un-initialized
instance of AVPacket.

This was the essence of my comment in ffmpeg_demux.cc where you had written

FfmpegDemuxImpl::DemuxPacket* demux_packet =
    new FfmpegDemuxImpl::DemuxPacket();

    av_init_packet(&demux_packet->av_packet);

and I asked you to put the init call in the constructor.

Now that you have, we are certain that the av_packet_ member must have been
properly initialized before the destructor runs, so the problem goes away.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:204:
demux_packet->presentation_time() + demux_packet->duration();
On 2016/03/01 20:43:01, dalesat wrote:
> On 2016/03/01 01:31:39, johngro wrote:
> > either PTS or duration may be AV_NOPTS_VALUE and need to be handled
> > appropriately.  See earlier comment.
> > 
> > In general, it would be nice to just eliminate the need for
> > next_presentation_time_.  Right now, it seems to exist only for the purpose
of
> > putting a timestamp on EOS payloads.
> > 
> > Finally, the demux seems to be putting timestamps on generated EOS payloads,
> it
> > does not seem like there should be a need to either generate or timestamp
EOS
> > payloads at the decoder level.
> 
> Acknowledged.

Great, if you agree that this can be an issue, but you are not going to fix it
in this go-around, can I please get a TODO so this does not get forgotten?

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:20:
std::unique_ptr<StreamType> StreamTypeFromAVCodecContext(AVCodecContext* from);
On 2016/03/01 20:43:02, dalesat wrote:
> On 2016/03/01 01:31:39, johngro wrote:
> > can we make AVCodecContext be a const unique_ptr<> & here (and a
unique_ptr<>
> in
> > the return from AVCodecContextFromStreamType below?)
> > 
> > Because of the custom deleter, it might be best to hoist the deleter functor
> out
> > of the FfmpegDecoderBase class and place it at a utility level.  Generally,
I
> > think it would be best to put its name in an internal {} namespace and
> > typedef/using the pointer type so that people don't have to write things
like
> > unique_ptr<AVCodecContext, internal::AVCodecContextDeleter>.
> > 
> > I know that Jeff disagrees somewhat with the typedef approach (argument; it
> > removes clarity about the underlying type of the pointer), but perhaps he
> would
> > be OK with it if it was something like
> > 
> > using AVCodecContextUPtr = ...;
> > 
> > or even
> > 
> > using AVCodecContextUniquePtr = ...;
> > 
> > My counter to Jeff's point would be that users of a unique pointer's class
> > should not know about details like custom deleters.  Also, if in attempting
to
> > avoid the need for knowledge of things like custom deleters, we end up
> returning
> > unmanaged pointers for newly created object from factory functions (things
> like
> > AVCodecContextFromStreamType below), then we are defeating the purpose of
> using
> > the managed pointer classes in the first place.
> 
> When a custom deleter is in play, I too prefer to define the pointer type.
> 
> Even if we're always managing Foo with a unique_ptr, I prefer passing const
Foo&
> to passing const std::unique_ptr<Foo>&.

Sure.

If the target function demands a non-null Foo pointer, then a const& is
definitely the way to go.  The burden is now on the caller to make sure that
their instances are never null before the "de-reference" them to pass into the
const& target.

If, based on the API, it is OK to pass null, then you end up needing a const
uptr<>&

https://codereview.chromium.org/1686363002/diff/120001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/120001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:22: using
AvCodecContextPtr =
Making this ContextUPtr (or even ContextUniquePtr) may head off criticism about
a lack of clarity as to what a ContextPtr is.

Then again, it might not.  You would need to get feedback from the people who
think that this style is unclear.

I'm fine with it either way.

[Sean Klein, 2016-03-01 22:19:52]

I've been added to this CL at the request of dalesat, since I added the other
"clang warning suppression" flags. I figured I'd respond here, so everyone could
see (and disagree with me, if necessary). 

I added those warning suppressions for two reasons: first, to update Clang,
which, with the addition of new warnings, was unable to compile multiple third
party libraries within Mojo. Secondly, to match parity with Chromium, which
chose to suppress the same warnings.

For any locally committed code, it's better to just not cause the warning at all
-- but specifically for third party code, which cannot be easily modified, I
would recommend modifying "cflags" (or "config") for the "framework_ffmpeg"
library. This way, if the warning really should be ignored within ffmpeg, it
isn't silenced for the rest of Mojo.

[Sean Klein, 2016-03-02 01:18:33]

Okay, I've been looking into this a bit more -- 

"cflags" / "public_configs" won't really work because they cannot apply cflag
modifications to a dependency. They can only alter higher level targets which
depend on the one including the suppressed warning.

However: you could potentially add the "-Wno-constant-conversion" flag to the
cflags field in the "no_chromium_code" config (in
build/config/compiler/BUILD.gn). This would only apply the warning suppression
to third party code.

[jamesr, 2016-03-02 01:19:26]

On 2016/03/02 at 01:18:33, smklein wrote:
> Okay, I've been looking into this a bit more -- 
> 
> "cflags" / "public_configs" won't really work because they cannot apply cflag
modifications to a dependency. They can only alter higher level targets which
depend on the one including the suppressed warning.

A public_config will apply the configuration to dependencies.  A config can
modify cflags.

> 
> However: you could potentially add the "-Wno-constant-conversion" flag to the
cflags field in the "no_chromium_code" config (in
build/config/compiler/BUILD.gn). This would only apply the warning suppression
to third party code.

[jamesr, 2016-03-02 01:20:33]

On 2016/03/02 at 01:19:26, jamesr wrote:
> On 2016/03/02 at 01:18:33, smklein wrote:
> > Okay, I've been looking into this a bit more -- 
> > 
> > "cflags" / "public_configs" won't really work because they cannot apply
cflag modifications to a dependency. They can only alter higher level targets
which depend on the one including the suppressed warning.
> 
> A public_config will apply the configuration to dependencies.  A config can
modify cflags.

Sorry, meant to say that a public_config will apply to *dependents*, which is
what you would want here assuming you apply it to the target that triggers the
warning either in its own code or its headers.

[Sean Klein, 2016-03-02 01:22:35]

Really? From the gn documentation:

public_configs: Configs to be applied on dependents.

  A list of config labels.

  Targets directly depending on this one will have the configs listed in
  this variable added to them. These configs will also apply to the
  current target.


^ This seems to imply that if A depends on B which depends on C, and "B"
includes cflag "-Wno-foo" in a public config, then the config will be applied to
"A" and "B", but not "C".

[jamesr, 2016-03-02 01:23:58]

That's right, and that's the behavior you generally want (my first comment was
backwards).  You would add a public_configs with "-Wno-foo" to "B" if target "B"
had code in a header that triggered -Wfoo

[Sean Klein, 2016-03-02 01:31:08]

I think the issue is that the code triggering the warning is
"third_party/ffmpeg/ffmpeg_internal", which is being DEPS-ed in.

Adding a public_config to services/media/framework_ffmpeg/BUILD.gn (the
equivalent of "B") will not change the third party code it depends on (the
equivalent of "C"). Changing the third party code would make things much easier
(perhaps the code causing the warning could be fixed), but I'm assuming we
aren't doing that.

If we're only modifying Mojo to land this change, then I would recommend
updating "no_chromium_code" -- it is a config already used by many DEPS-ed in
third_party repos, and it will not suppress warnings for any Mojo code.

[jamesr, 2016-03-02 02:03:50]

Then we need to DEPS in from something we control (otherwise I'm not sure what
the proposed path to fixing this warning at all is).

"no_chromium_code" is a better short term option for sure, but it doesn't answer
how we remove the suppression down the line.

[jamesr, 2016-03-02 02:19:23]

Secondary used to work the way you're thinking but was switched to prefer the
natural location if it exists when enough things converted over.

[dalesat, 2016-03-02 18:35:36]

PTAL...build/config/compiler/BUILD.gn changed as discussed.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_demux.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:56: av_free_packet(&av_packet);
On 2016/03/01 22:07:26, johngro wrote:
> On 2016/03/01 20:43:02, dalesat wrote:
> > On 2016/03/01 01:31:39, johngro wrote:
> > > what happens if the av_packet was never allocated by ffmpeg?
> > 
> > Not sure what you mean. This packet implementation is specifically for
> wrapping
> > av_packet.
> 
> yes, but nothing in this class made certain that av_packet_ had been properly
> initialized.  Before, if an instance of the class was created then immediately
> destroyed, av_free_packet was going to get called with an un-initialized
> instance of AVPacket.
> 
> This was the essence of my comment in ffmpeg_demux.cc where you had written
> 
> FfmpegDemuxImpl::DemuxPacket* demux_packet =
>     new FfmpegDemuxImpl::DemuxPacket();
> 
>     av_init_packet(&demux_packet->av_packet);
> 
> and I asked you to put the init call in the constructor.
> 
> Now that you have, we are certain that the av_packet_ member must have been
> properly initialized before the destructor runs, so the problem goes away.

Acknowledged.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_demux.cc:204:
demux_packet->presentation_time() + demux_packet->duration();
On 2016/03/01 22:07:26, johngro wrote:
> On 2016/03/01 20:43:01, dalesat wrote:
> > On 2016/03/01 01:31:39, johngro wrote:
> > > either PTS or duration may be AV_NOPTS_VALUE and need to be handled
> > > appropriately.  See earlier comment.
> > > 
> > > In general, it would be nice to just eliminate the need for
> > > next_presentation_time_.  Right now, it seems to exist only for the
purpose
> of
> > > putting a timestamp on EOS payloads.
> > > 
> > > Finally, the demux seems to be putting timestamps on generated EOS
payloads,
> > it
> > > does not seem like there should be a need to either generate or timestamp
> EOS
> > > payloads at the decoder level.
> > 
> > Acknowledged.
> 
> Great, if you agree that this can be an issue, but you are not going to fix it
> in this go-around, can I please get a TODO so this does not get forgotten?

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:20:
std::unique_ptr<StreamType> StreamTypeFromAVCodecContext(AVCodecContext* from);
On 2016/03/01 22:07:26, johngro wrote:
> On 2016/03/01 20:43:02, dalesat wrote:
> > On 2016/03/01 01:31:39, johngro wrote:
> > > can we make AVCodecContext be a const unique_ptr<> & here (and a
> unique_ptr<>
> > in
> > > the return from AVCodecContextFromStreamType below?)
> > > 
> > > Because of the custom deleter, it might be best to hoist the deleter
functor
> > out
> > > of the FfmpegDecoderBase class and place it at a utility level. 
Generally,
> I
> > > think it would be best to put its name in an internal {} namespace and
> > > typedef/using the pointer type so that people don't have to write things
> like
> > > unique_ptr<AVCodecContext, internal::AVCodecContextDeleter>.
> > > 
> > > I know that Jeff disagrees somewhat with the typedef approach (argument;
it
> > > removes clarity about the underlying type of the pointer), but perhaps he
> > would
> > > be OK with it if it was something like
> > > 
> > > using AVCodecContextUPtr = ...;
> > > 
> > > or even
> > > 
> > > using AVCodecContextUniquePtr = ...;
> > > 
> > > My counter to Jeff's point would be that users of a unique pointer's class
> > > should not know about details like custom deleters.  Also, if in
attempting
> to
> > > avoid the need for knowledge of things like custom deleters, we end up
> > returning
> > > unmanaged pointers for newly created object from factory functions (things
> > like
> > > AVCodecContextFromStreamType below), then we are defeating the purpose of
> > using
> > > the managed pointer classes in the first place.
> > 
> > When a custom deleter is in play, I too prefer to define the pointer type.
> > 
> > Even if we're always managing Foo with a unique_ptr, I prefer passing const
> Foo&
> > to passing const std::unique_ptr<Foo>&.
> 
> Sure.
> 
> If the target function demands a non-null Foo pointer, then a const& is
> definitely the way to go.  The burden is now on the caller to make sure that
> their instances are never null before the "de-reference" them to pass into the
> const& target.
> 
> If, based on the API, it is OK to pass null, then you end up needing a const
> uptr<>&

Acknowledged.

[jamesr, 2016-03-02 18:45:30]

The //build change looks OK, but could you file a bug (and add a link to it in
the comment) to keep track of the work needed to remove this?  What's your plan?

[dalesat, 2016-03-02 19:39:01]

Done

[dalesat, 2016-03-03 01:30:22]

Ping! LGTM, anyone?

[dalesat, 2016-03-03 20:41:11]

PTAL...all comments addressed.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:18: if
(av_sample_fmt_is_planar(av_codec_context->sample_fmt)) {
On 2016/03/01 18:05:35, johngro wrote:
> DCHECK(av_codec_context_->channels);
> 
> There is a non-zero assumption made for this field in the planar path of
> Allocate below.

Done.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:99: payload_buffer =
av_frame_->data[0];
On 2016/03/01 18:05:35, johngro wrote:
> So, I think that ffmpeg is going to end up freeing your buffer out from under
> you in the interleaved case.  The sequence would be..
> 
> 1) During decode, ffmpeg calls your AllocateBufferForAvFrame hook
> 2) A single buffer is allocated using self->allocator_.
> 2.1) av_frame_->data[0] now points to this buffer.
> 2.2) the call to av_buffer_create also holds a pointer to the buffer as well
as
> the hook for ReleaseBufferForAvFrame.
> 3) After decode, framework calls PacketFromAvFrame.
> 3.1) PacketFromAvFrame calls CreateOutputPacket (here in the code), and you
grab
> a pointer to the buffer, but do not detach the buffer from the av_frame_
> instance.
> 3.2) Packet::Create is called and passed a pointer to the buffer.
> 3.3) PacketFromAvFrame calls av_frame_unref.
> 3.4) av_frame_unref eventually calls the ReleaseBufferHook which frees the
> buffer.
> 4) The packet is sent down the line.  It is holding a dead pointer. 
Eventually
> (assuming it has not already crashed) it will double-free this pointer.

This code actually works (ReleaseBufferForAvFrame won't free the buffer because
allocator_ == nullptr), but I'm not happy with it. I wrote it some months ago,
and I had trouble figuring out why it works. I've defined a new class called
AvBufferContext, which I'm using as the opaque context for AVBuffer.
AvBufferContext retains the buffer pointer, buffer size and allocator. Its
Release method releases ownership of the buffer so it won't get released in
ReleaseBufferForAvFrame.

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:133: if (buffer_size <
0) {
On 2016/03/01 18:05:35, johngro wrote:
> <= 0
> 
> Also, need to make sure that the rest of the code will produce coherent
results
> if you shortcut at this point (av_buffer_create is not going to be called if
you
> bail out now, for example).
> 
> That, or make sure the code below will tolerate a 0 buffer size.  Right now,
> AllocatePayloadBuffer in the default allocator will not tolerate a request to
> allocate 0 bytes.

Opted to tolerate buffer_size == 0

https://codereview.chromium.org/1686363002/diff/100001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:185: // We lose the
buffer above before CreatePacket gets called, so we save the
On 2016/03/01 18:05:35, johngro wrote:
> Why do we lose the buffer?  Isn't the pointer to it stored in
av_frame->buf[0]? 
> It is not an opaque data structure, you should just be able to use the
size/data
> members of av_frame->buf[0] instead of needing to stash values in the class.

Done.

https://codereview.chromium.org/1686363002/diff/120001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_type_converters.h (right):

https://codereview.chromium.org/1686363002/diff/120001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_type_converters.h:22: using
AvCodecContextPtr =
On 2016/03/01 22:07:26, johngro wrote:
> Making this ContextUPtr (or even ContextUniquePtr) may head off criticism
about
> a lack of clarity as to what a ContextPtr is.
> 
> Then again, it might not.  You would need to get feedback from the people who
> think that this style is unclear.
> 
> I'm fine with it either way.

Acknowledged.

[johngro, 2016-03-03 23:03:16]

TL;DR

I'm still a bit uneasy, but let's not hold this up any longer.

LGTM

https://codereview.chromium.org/1686363002/diff/220001/services/media/framewo...
File services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1686363002/diff/220001/services/media/framewo...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:197: // or it's the same
as the buffer parameter.
this still makes me vaguely uneasy...  The FFmpeg frame is still holding a
pointer to the buffer which once lived in the av_buffer_context but may have
been detached.

The safety of all of this depends on being certain that after we detach the
buffer from the context, that the FFmpeg never makes any more attempts to do
anything with the frame which still has a copy of the buffer pointer.

In this case, we should have this guarantee, because
FfmpegDecoderBase::PacketFromAvFrame is going to call av_frame_unref immediately
following the call to CreateOutputPacket; but this is not immediately obvious at
the point that we detach the buffer from the buffer context (line 94 above).

Another way to approch this might have been to actually go into the AVFrame
structure and null out the release hook, the release hook context, and the
buffer/length pointers in the frame->buf[*] members if you take ownership of the
memory back from the frame.  Then, no matter what happens with the frame from
there on out, it will not be able to touch the memory you have taken back from
it.  It has the added benefit of eliminating the need to heap allocate the
context structure for each frame

I don't want to hold this up any longer.  I'm going to open an issue to track my
uneasy-ness which I will CC you on.  We can get this in and come back later to
clean up the ownership interactions with FFmpeg.

[dalesat, 2016-03-03 23:45:08]

Description was changed from

==========
Motown: ffmpeg implementations of framework 'parts'
ffmpeg_video_decoder.* is not yet functional
==========

to

==========
Motown: ffmpeg implementations of framework 'parts'
ffmpeg_video_decoder.* is not yet functional

R=johngro@google.com

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

[dalesat, 2016-03-03 23:45:10]

Committed patchset #12 (id:220001) manually as
ccde3cc6961adbb482d6aea82ed2f9c07959b9bc (presubmit successful).