Introduce some buffering into the cast media pipeline.

Introduce some buffering into the cast media pipeline.

BufferingFrameProvider is a CodedFrameProvider which buffers frames
up to a given level.

BUG=408189
Committed: https://crrev.com/1f24028e02a16438719a9b406abd03f58c074f1a
Cr-Commit-Position: refs/heads/master@{#294054}

[damienv1, 2014-09-09 01:45:23]

damienv@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org, scherkus@chromium.org,
xhwang@chromium.org

[damienv1, 2014-09-09 15:36:26]

Please take a look. Thanks.

[gunsch, 2014-09-09 15:58:19]

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
File chromecast/media/cma/base/buffering_frame_provider.cc (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:41: weak_factory_(new
base::WeakPtrFactory<BufferingFrameProvider>(this)) {
weak_factory_(this)

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:43: weak_this_ =
weak_factory_->GetWeakPtr();
since initializers are executed in order, you should be able to add this as a
member initializer above

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:97: // then we consider
the max buffering capacity as reached.
nit: I think we've been chided for using "we" in comments before :)

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:99: (total_buffer_size_ +
max_frame_size_ >= max_buffer_size_);
nit: unnecessary parens

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
File chromecast/media/cma/base/buffering_frame_provider.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:24: //
remove

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:30: // Create a frame
provider that buffers coded frames up to the
nit: Creates

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:109:
scoped_ptr<base::WeakPtrFactory<BufferingFrameProvider> > weak_factory_;
It seems to be more typical to have a member base::WeakPtrFactory that is not a
pointer.

Ref:
https://code.google.com/p/chromium/codesearch#search/&q=base::weakptrfactory%...

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
File chromecast/media/cma/base/frame_generator_for_test.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
chromecast/media/cma/base/frame_generator_for_test.h:36: // Indicate whether the
next frame should come with a new decoder config.
nit: Indicates

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
chromecast/media/cma/base/frame_generator_for_test.h:39: // Generate a frame.
nit: Generates

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
File chromecast/media/cma/base/mock_frame_provider.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
chromecast/media/cma/base/mock_frame_provider.h:33: // Parametrization of the
frame provider.
nit: Parameterization

[damienv1, 2014-09-09 16:24:18]

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
File chromecast/media/cma/base/buffering_frame_provider.cc (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:41: weak_factory_(new
base::WeakPtrFactory<BufferingFrameProvider>(this)) {
On 2014/09/09 15:58:18, gunsch wrote:
> weak_factory_(this)

I am using a pointer in this case (to be able to re-create a weak pointer
factory later in Flush).

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:43: weak_this_ =
weak_factory_->GetWeakPtr();
On 2014/09/09 15:58:18, gunsch wrote:
> since initializers are executed in order, you should be able to add this as a
> member initializer above

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:97: // then we consider
the max buffering capacity as reached.
On 2014/09/09 15:58:18, gunsch wrote:
> nit: I think we've been chided for using "we" in comments before :)

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.cc:99: (total_buffer_size_ +
max_frame_size_ >= max_buffer_size_);
On 2014/09/09 15:58:18, gunsch wrote:
> nit: unnecessary parens

I find it easier to read though.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
File chromecast/media/cma/base/buffering_frame_provider.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:24: //
On 2014/09/09 15:58:19, gunsch wrote:
> remove

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:30: // Create a frame
provider that buffers coded frames up to the
On 2014/09/09 15:58:19, gunsch wrote:
> nit: Creates

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
chromecast/media/cma/base/buffering_frame_provider.h:109:
scoped_ptr<base::WeakPtrFactory<BufferingFrameProvider> > weak_factory_;
On 2014/09/09 15:58:19, gunsch wrote:
> It seems to be more typical to have a member base::WeakPtrFactory that is not
a
> pointer.
> 
> Ref:
>
https://code.google.com/p/chromium/codesearch#search/&q=base::weakptrfactory%...

This one is a special case since I have to re-create a weak pointer in Flush.
Since there is no copy constructor, the weak pointer factory must be a pointer
in this case (Please let me know if you have an alternate solution without using
pointers).

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
File chromecast/media/cma/base/frame_generator_for_test.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
chromecast/media/cma/base/frame_generator_for_test.h:36: // Indicate whether the
next frame should come with a new decoder config.
On 2014/09/09 15:58:19, gunsch wrote:
> nit: Indicates

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
chromecast/media/cma/base/frame_generator_for_test.h:39: // Generate a frame.
On 2014/09/09 15:58:19, gunsch wrote:
> nit: Generates

Done.

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
File chromecast/media/cma/base/mock_frame_provider.h (right):

https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
chromecast/media/cma/base/mock_frame_provider.h:33: // Parametrization of the
frame provider.
On 2014/09/09 15:58:19, gunsch wrote:
> nit: Parameterization

Done.

[gunsch, 2014-09-09 20:11:03]

On 2014/09/09 16:24:18, damienv1 wrote:
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> File chromecast/media/cma/base/buffering_frame_provider.cc (right):
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.cc:41: weak_factory_(new
> base::WeakPtrFactory<BufferingFrameProvider>(this)) {
> On 2014/09/09 15:58:18, gunsch wrote:
> > weak_factory_(this)
> 
> I am using a pointer in this case (to be able to re-create a weak pointer
> factory later in Flush).
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.cc:43: weak_this_ =
> weak_factory_->GetWeakPtr();
> On 2014/09/09 15:58:18, gunsch wrote:
> > since initializers are executed in order, you should be able to add this as
a
> > member initializer above
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.cc:97: // then we consider
> the max buffering capacity as reached.
> On 2014/09/09 15:58:18, gunsch wrote:
> > nit: I think we've been chided for using "we" in comments before :)
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.cc:99: (total_buffer_size_
+
> max_frame_size_ >= max_buffer_size_);
> On 2014/09/09 15:58:18, gunsch wrote:
> > nit: unnecessary parens
> 
> I find it easier to read though.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> File chromecast/media/cma/base/buffering_frame_provider.h (right):
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.h:24: //
> On 2014/09/09 15:58:19, gunsch wrote:
> > remove
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.h:30: // Create a frame
> provider that buffers coded frames up to the
> On 2014/09/09 15:58:19, gunsch wrote:
> > nit: Creates
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/bu...
> chromecast/media/cma/base/buffering_frame_provider.h:109:
> scoped_ptr<base::WeakPtrFactory<BufferingFrameProvider> > weak_factory_;
> On 2014/09/09 15:58:19, gunsch wrote:
> > It seems to be more typical to have a member base::WeakPtrFactory that is
not
> a
> > pointer.
> > 
> > Ref:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=base::weakptrfactory%...
> 
> This one is a special case since I have to re-create a weak pointer in Flush.
> Since there is no copy constructor, the weak pointer factory must be a pointer
> in this case (Please let me know if you have an alternate solution without
using
> pointers).
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
> File chromecast/media/cma/base/frame_generator_for_test.h (right):
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
> chromecast/media/cma/base/frame_generator_for_test.h:36: // Indicate whether
the
> next frame should come with a new decoder config.
> On 2014/09/09 15:58:19, gunsch wrote:
> > nit: Indicates
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/fr...
> chromecast/media/cma/base/frame_generator_for_test.h:39: // Generate a frame.
> On 2014/09/09 15:58:19, gunsch wrote:
> > nit: Generates
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
> File chromecast/media/cma/base/mock_frame_provider.h (right):
> 
>
https://codereview.chromium.org/554893003/diff/1/chromecast/media/cma/base/mo...
> chromecast/media/cma/base/mock_frame_provider.h:33: // Parametrization of the
> frame provider.
> On 2014/09/09 15:58:19, gunsch wrote:
> > nit: Parameterization
> 
> Done.

lgtm % a couple nits

[gunsch, 2014-09-09 20:11:11]

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_frame_provider.cc (right):

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_frame_provider.cc:67: // This is needed
since even though, |coded_frame_provider_| is flushed later
Comma nit: "This is needed since, even though |coded_frame_provider|..."

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_frame_provider.cc:70:
weak_factory_.reset(new base::WeakPtrFactory<BufferingFrameProvider>(this));
do you need to actually reset this? shouldn't InvalidateWeakPtrs be sufficient?

[damienv1, 2014-09-09 20:29:17]

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_frame_provider.cc (right):

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_frame_provider.cc:67: // This is needed
since even though, |coded_frame_provider_| is flushed later
On 2014/09/09 20:11:11, gunsch wrote:
> Comma nit: "This is needed since, even though |coded_frame_provider|..."

Done.

https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_frame_provider.cc:70:
weak_factory_.reset(new base::WeakPtrFactory<BufferingFrameProvider>(this));
On 2014/09/09 20:11:11, gunsch wrote:
> do you need to actually reset this? shouldn't InvalidateWeakPtrs be
sufficient?

Yes, it should be enough.
Somehow, I believed this was not enough (Invalidating only applies to previous
weak pointers, that's what I missed).

[gunsch, 2014-09-09 20:34:34]

On 2014/09/09 20:29:17, damienv1 wrote:
>
https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
> File chromecast/media/cma/base/buffering_frame_provider.cc (right):
> 
>
https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
> chromecast/media/cma/base/buffering_frame_provider.cc:67: // This is needed
> since even though, |coded_frame_provider_| is flushed later
> On 2014/09/09 20:11:11, gunsch wrote:
> > Comma nit: "This is needed since, even though |coded_frame_provider|..."
> 
> Done.
> 
>
https://codereview.chromium.org/554893003/diff/20001/chromecast/media/cma/bas...
> chromecast/media/cma/base/buffering_frame_provider.cc:70:
> weak_factory_.reset(new base::WeakPtrFactory<BufferingFrameProvider>(this));
> On 2014/09/09 20:11:11, gunsch wrote:
> > do you need to actually reset this? shouldn't InvalidateWeakPtrs be
> sufficient?
> 
> Yes, it should be enough.
> Somehow, I believed this was not enough (Invalidating only applies to previous
> weak pointers, that's what I missed).

lgtm

[lcwu1, 2014-09-09 23:27:06]

lgtm

[damienv1, 2014-09-09 23:55:44]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:07:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/554893003/40001

[commit-bot: I haz the power, 2014-09-10 00:43:34]

Committed patchset #3 (id:40001) as 1b24b2283b9783e95a9ba5b440e3f57fd6e1c092

[commit-bot: I haz the power, 2014-09-10 03:57:03]

Patchset 3 (id:??) landed as
https://crrev.com/1f24028e02a16438719a9b406abd03f58c074f1a
Cr-Commit-Position: refs/heads/master@{#294054}