Motown: Improvements to packet definition

Motown: Improvements to packet definition
- Removed duration field
- Renamed various "PresentationTime" things to "Pts"
- Now implementing Packet accessors in the base class.
- Improved the way ffmpeg AVFrame and AVPacket structures are managed.

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/103f68e416fc15e16dab1ff20af2af5b1b5e01c1

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

Description was changed from

==========
Motown: Improvements to packet definition
- Removed duration field
- Renamed various "PresentationTime" things to "Pts"
- Now implementing Packet accessors in the base class.
- Improved the way ffmpeg AVFrame and AVPacket structures are managed.
==========

to

==========
Motown: Improvements to packet definition
- Removed duration field
- Renamed various "PresentationTime" things to "Pts"
- Now implementing Packet accessors in the base class.
- Improved the way ffmpeg AVFrame and AVPacket structures are managed.
==========

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

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

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

Please review. Thanks!

[johngro, 2016-03-21 19:47:56]

small stuff only

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
File services/media/framework/packet.cc (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
services/media/framework/packet.cc:39: if (payload() != nullptr && allocator_ !=
nullptr) {
we lost the DCHECK(allocator_) here.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:279: DCHECK(bytes_per_frame() != 0);
the DCHECK which checks bytes_per_frame should come before the DCHECK which uses
bytes_per_frame in a div/mod operation.

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
File services/media/framework_ffmpeg/av_packet.h (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_packet.h:14: namespace media {
just a suggestion (take it or leave it); As these helpers (AVFrame, AVPacket,
etc) are specific to FFmpeg, and use some of the same terminology which we use
elsewhere (we also call access-unit-like things "packets") we might want to
stuff these definitions into a mojo::media::ffmpeg namespace.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:46: next_pts_ = 0;
I'm still a bit confused by this logic; eg, why is the ffmpeg decoder wrapper
making timestamp interpolation decisions instead of just putting the timestamps
produced by ffmpeg onto the payloads during CreateOutputPacket?

No action is being requested here.  Just noting that it is something we need to
sit down and discuss at some point.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_demux.cc:74: av_packet->pts,
Since this is a place where we we are coming from the FFmpeg world into our
world, we should take care to translate the unknown PTS sentinel value.

IOW, this should be

(av_packet->pts == AV_NOPTS_VALUE) ? kUnknownPts : av_packet->pts),

If we have pegged these to the same value, then we should add a
static_assert(AV_NOPTS_VALUE == kUnknownPts) (Immediately before the constructor
declaration is probably the best place).

Having a DCHECK to make sure that av_packet is non-null before accessing it
would be nice as well, but I am reluctant to ask you to re-write your
initialization list as an in-body initialization.

Finally, a style nit which I don't care about (take it or leave it), we are
supposed to drop the : for initializer lists.

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_demux.cc:76:
static_cast<size_t>(av_packet->size),
If this cast is required because av_packet->size is signed, it would be good to
DCHECK(av_packet->size >= 0) somewhere in the constructor body.

[dalesat, 2016-03-21 22:29:45]

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

[dalesat, 2016-03-21 22:30:59]

PTAL

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
File services/media/framework/packet.cc (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
services/media/framework/packet.cc:39: if (payload() != nullptr && allocator_ !=
nullptr) {
On 2016/03/21 19:47:56, johngro wrote:
> we lost the DCHECK(allocator_) here.

It seemed unnecessary - the previous line checks that it's not null.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:279: DCHECK(bytes_per_frame() != 0);
On 2016/03/21 19:47:56, johngro wrote:
> the DCHECK which checks bytes_per_frame should come before the DCHECK which
uses
> bytes_per_frame in a div/mod operation.

Done.

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
File services/media/framework_ffmpeg/av_packet.h (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/av_packet.h:14: namespace media {
On 2016/03/21 19:47:56, johngro wrote:
> just a suggestion (take it or leave it); As these helpers (AVFrame, AVPacket,
> etc) are specific to FFmpeg, and use some of the same terminology which we use
> elsewhere (we also call access-unit-like things "packets") we might want to
> stuff these definitions into a mojo::media::ffmpeg namespace.

Done.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc:46: next_pts_ = 0;
On 2016/03/21 19:47:56, johngro wrote:
> I'm still a bit confused by this logic; eg, why is the ffmpeg decoder wrapper
> making timestamp interpolation decisions instead of just putting the
timestamps
> produced by ffmpeg onto the payloads during CreateOutputPacket?
> 
> No action is being requested here.  Just noting that it is something we need
to
> sit down and discuss at some point.

Acknowledged.

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

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_demux.cc:74: av_packet->pts,
On 2016/03/21 19:47:56, johngro wrote:
> Since this is a place where we we are coming from the FFmpeg world into our
> world, we should take care to translate the unknown PTS sentinel value.
> 
> IOW, this should be
> 
> (av_packet->pts == AV_NOPTS_VALUE) ? kUnknownPts : av_packet->pts),
> 
> If we have pegged these to the same value, then we should add a
> static_assert(AV_NOPTS_VALUE == kUnknownPts) (Immediately before the
constructor
> declaration is probably the best place).
> 
> Having a DCHECK to make sure that av_packet is non-null before accessing it
> would be nice as well, but I am reluctant to ask you to re-write your
> initialization list as an in-body initialization.
> 
> Finally, a style nit which I don't care about (take it or leave it), we are
> supposed to drop the : for initializer lists.

Done.

https://codereview.chromium.org/1814553002/diff/1/services/media/framework_ff...
services/media/framework_ffmpeg/ffmpeg_demux.cc:76:
static_cast<size_t>(av_packet->size),
On 2016/03/21 19:47:56, johngro wrote:
> If this cast is required because av_packet->size is signed, it would be good
to
> DCHECK(av_packet->size >= 0) somewhere in the constructor body.

Done.

[johngro, 2016-03-21 22:56:06]

lgtm
one clarification question only which can be responded to OOB if the code is OK
as it stands.

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
File services/media/framework/packet.cc (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
services/media/framework/packet.cc:39: if (payload() != nullptr && allocator_ !=
nullptr) {
On 2016/03/21 22:30:58, dalesat wrote:
> On 2016/03/21 19:47:56, johngro wrote:
> > we lost the DCHECK(allocator_) here.
> 
> It seemed unnecessary - the previous line checks that it's not null.

fair enough.  What is the intended behavior if payload is non-null and allocator
is nullptr?  I had assumed that this was a mistake and that the intention was to
say something like

if (playload) {
  assert(allocator);
  allocator->release(payload);
}

[dalesat, 2016-03-22 00:06:56]

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
File services/media/framework/packet.cc (right):

https://codereview.chromium.org/1814553002/diff/1/services/media/framework/pa...
services/media/framework/packet.cc:39: if (payload() != nullptr && allocator_ !=
nullptr) {
On 2016/03/21 22:56:05, johngro wrote:
> On 2016/03/21 22:30:58, dalesat wrote:
> > On 2016/03/21 19:47:56, johngro wrote:
> > > we lost the DCHECK(allocator_) here.
> > 
> > It seemed unnecessary - the previous line checks that it's not null.
> 
> fair enough.  What is the intended behavior if payload is non-null and
allocator
> is nullptr?  I had assumed that this was a mistake and that the intention was
to
> say something like
> 
> if (playload) {
>   assert(allocator);
>   allocator->release(payload);
> }

The code is correct but unclear. Added a comment to explain.

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

Description was changed from

==========
Motown: Improvements to packet definition
- Removed duration field
- Renamed various "PresentationTime" things to "Pts"
- Now implementing Packet accessors in the base class.
- Improved the way ffmpeg AVFrame and AVPacket structures are managed.
==========

to

==========
Motown: Improvements to packet definition
- Removed duration field
- Renamed various "PresentationTime" things to "Pts"
- Now implementing Packet accessors in the base class.
- Improved the way ffmpeg AVFrame and AVPacket structures are managed.

R=johngro@google.com

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

[dalesat, 2016-03-22 00:07:38]

Committed patchset #3 (id:40001) manually as
103f68e416fc15e16dab1ff20af2af5b1b5e01c1 (presubmit successful).