[Cast] VideoFrameFactory interface to vend frames with encoder affinity.

[Cast] VideoFrameFactory interface to vend frames with encoder affinity.

Certain encoder implementations may provide memory buffers with special
properties in which clients are expected to write input frames. This
patch introduces a simple new interface allowing a VideoEncoder to
provide a factory object from which clients may obtain input video
frames. Encoder implementations are free to return null, in which case
VideoSender provides a default implementation based on VideoFramePool.

The upcoming VideoToolbox encoder implementation for OS X and iOS will
provide a custom implementation of this interface that will use the
encoder's underlying CVPixelBufferPool. The buffers vended by that pool
are backed by memory shared between the encoding process and the daemon
interfacing with the hardware video encoder. Clients that write into
these buffers will avoid a full frame copy of every input frame, which
is a substantial saving in memory bandwidth and latency for 1080p@60
video.

BUG=429057
R=miu, hclam, hubbe

Committed: https://crrev.com/dcff178ab70511a4135895bc092ad3b871a0ae3d
Cr-Commit-Position: refs/heads/master@{#304973}

[jfroy, 2014-10-31 02:03:55]

This patch adds infrastructure to allow video encoders to vend optimal buffers
into which input frames can be written. This will enable an important
optimization for the VideoToolbox encoder on iOS without relying on hacks or
special knowledge on the part of the client.

[jfroy, 2014-11-14 18:21:08]

jfroy@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sandersd@chromium.org

[DaleCurtis, 2014-11-14 19:09:48]

This is all cast/ code, so it's really up to you guys to figure out your plan
and what's acceptable to you. I'm happy to look through, but cc:sergeyu,tommi
from Chromoting and WebRTC who might have a stronger interest in this code
pattern.

[jfroy, 2014-11-14 19:22:31]

On 2014/11/14 19:09:48, DaleCurtis wrote:
> This is all cast/ code, so it's really up to you guys to figure out your plan
> and what's acceptable to you. I'm happy to look through, but cc:sergeyu,tommi
> from Chromoting and WebRTC who might have a stronger interest in this code
> pattern.

I've added media owners because of media/media.gyp changes. I'll need a lgtm
from one of you to eventually submit this.

[hubbe, 2014-11-14 19:22:54]

On 2014/11/14 18:21:08, jfroy wrote:

I think it's good, I'll let hclam review it as well though.
One question though: How does the video encoder know if the caller used the
factory to create the video frames or not?

[jfroy, 2014-11-14 19:31:46]

On 2014/11/14 19:22:54, hubbe wrote:
> On 2014/11/14 18:21:08, jfroy wrote:
> 
> I think it's good, I'll let hclam review it as well though.
> One question though: How does the video encoder know if the caller used the
> factory to create the video frames or not?

An encoder can't know just looking at the VideoFrame object itself. It
presumably can find out by looking at the data pointers from the video frame.
Encoders should generally accept any VideoFrame, but offer optimized performance
if a VideoFrame with affinity (i.e. a VideoFrame vended by its
VideoFrameFactory) is submitted.

For example, my VideoToolbox encoder will implement a specialized
VideoFrameFactory that will vend VideoFrame objects backed by buffers obtained
from the encoder's pixel buffer pool. Those buffers reside in special shared
memory segments visible to the encoding process and hardware encoder (apps do
not have direct access to the hardware encoder; encoding is mediated by a system
daemon). If a generic VideoFrame is submitted, a data copy will occur inside
VideoToolbox. If a VideoFrame from that VideoFrameFactory is submitted, that
copy will be eliminated. Either way, things work. You can make them work
potentially faster if you adopt this API.

There is something to be said about clients preferring their own VideoFrame
pooling / management system. I guess it will be up to each cast sender app /
subsystem to choose what to use.

[Alpha Left Google, 2014-11-17 20:23:13]

hclam@chromium.org changed reviewers:
+ hclam@chromium.org

[Alpha Left Google, 2014-11-17 20:23:14]

I see some issues related to threading. I think it is dangerous to create a
VideoFrameFactory on cast main thread; use it on any thread; and it communicates
with encoder on encoder thread to create most optimized VideoFrame. It might
work for OSX but might not scale to other implementations. All this threading
logic should be hidden inside the implementation of VideoEncoder.

Second issue is how does the client of CastSender knows when to use this API? In
Chrome there's certainly no need to call CreateFrame() and it creates confusion.

What about using this interface?

class VideoSender {

  // Returns true if an optimized version of VideoFrame is
  // provided. Client of this API should call
  // CreateOptimizedVideoFrame() to create such VideoFrames
  // and submit them for encoding.
  bool ShouldUseOptimizedVideoFrame();

  // Create an optimized version of VideoFrame asynchronously.
  void CreateOptimizedVideoFrame(
    base::TimeDelta timestamp,
    const base::Callback<media::VideoFrame>& video_frame_cb 
  );
};

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h...
media/cast/cast_sender.h:42: virtual scoped_refptr<VideoFrame>
CreateFrame(base::TimeDelta timestamp) = 0;
What if CastSender is not yet initialized? Then there won't be an encoder and
this method wouldn't know what kind of frames to create. Code change is not
needed, I think you just need to clarify in comments.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:38: return
video_frame_factory_->CreateFrame(timestamp);
Why not create VideoFrame through VideoSender?

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:49: scoped_refptr<VideoFrameFactory>
video_frame_factory_;
Please don't save VideoFrameFactory. It's an internal member of VideoSender. You
can already access VidoeFrameFactory through VideoSender or VideoSender can
create a VideoFrame using internal objects.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder.h:49: virtual scoped_refptr<VideoFrameFactory>
GetVideoFrameFactory() = 0;
I don't think the threading model for this method is good.

According to this interface. VideoframeFactory is going to be called on the main
thread, while encoder is running on the encoder thread. Which means
VideoFrameFactory will have to perform cross thread communication with encoder
implementation. It is easy to make mistakes if each encoder & VideoFrameFactory
has to implement this. And it violates the design that VideoEncoder is
sinlge-threaded.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory.h:20: class VideoFrameFactory : public
base::RefCountedThreadSafe<VideoFrameFactory> {
I think it's unrealistic that this class is not thread safe, but can be used on
any thread and communicates with encoder to creates the most performant
VideoFrame. This just means internally it has some cross thread communication
with the encoder or encoder thread.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory_pool_impl.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:18: // Implementation of the
VideoFrameFactory interface using |VideoFramePool|.
I see no user of this class other than unit test. Do we really need this? Are we
going to use it?

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:32: VideoFramePool pool_;
There's really not need for a software based VideoFrame pool. If it is unit test
only it should only be in the test.

[hubbe, 2014-11-17 20:28:22]

On 2014/11/14 19:31:46, jfroy wrote:
> On 2014/11/14 19:22:54, hubbe wrote:
> > On 2014/11/14 18:21:08, jfroy wrote:
> > 
> > I think it's good, I'll let hclam review it as well though.
> > One question though: How does the video encoder know if the caller used the
> > factory to create the video frames or not?
> 
> An encoder can't know just looking at the VideoFrame object itself. It
> presumably can find out by looking at the data pointers from the video frame.
> Encoders should generally accept any VideoFrame, but offer optimized
performance
> if a VideoFrame with affinity (i.e. a VideoFrame vended by its
> VideoFrameFactory) is submitted.
> 
> For example, my VideoToolbox encoder will implement a specialized
> VideoFrameFactory that will vend VideoFrame objects backed by buffers obtained
> from the encoder's pixel buffer pool. Those buffers reside in special shared
> memory segments visible to the encoding process and hardware encoder (apps do
> not have direct access to the hardware encoder; encoding is mediated by a
system
> daemon). If a generic VideoFrame is submitted, a data copy will occur inside
> VideoToolbox. If a VideoFrame from that VideoFrameFactory is submitted, that
> copy will be eliminated. Either way, things work. You can make them work
> potentially faster if you adopt this API.
> 
> There is something to be said about clients preferring their own VideoFrame
> pooling / management system. I guess it will be up to each cast sender app /
> subsystem to choose what to use.

You said "if a generic VideoFrame is submitted..." but how does it know if it is
generic or not?

[jfroy, 2014-11-17 20:48:47]

On 2014/11/17 20:28:22, hubbe wrote:
> On 2014/11/14 19:31:46, jfroy wrote:
> > On 2014/11/14 19:22:54, hubbe wrote:
> > > On 2014/11/14 18:21:08, jfroy wrote:
> > > 
> > > I think it's good, I'll let hclam review it as well though.
> > > One question though: How does the video encoder know if the caller used
the
> > > factory to create the video frames or not?
> > 
> > An encoder can't know just looking at the VideoFrame object itself. It
> > presumably can find out by looking at the data pointers from the video
frame.
> > Encoders should generally accept any VideoFrame, but offer optimized
> performance
> > if a VideoFrame with affinity (i.e. a VideoFrame vended by its
> > VideoFrameFactory) is submitted.
> > 
> > For example, my VideoToolbox encoder will implement a specialized
> > VideoFrameFactory that will vend VideoFrame objects backed by buffers
obtained
> > from the encoder's pixel buffer pool. Those buffers reside in special shared
> > memory segments visible to the encoding process and hardware encoder (apps
do
> > not have direct access to the hardware encoder; encoding is mediated by a
> system
> > daemon). If a generic VideoFrame is submitted, a data copy will occur inside
> > VideoToolbox. If a VideoFrame from that VideoFrameFactory is submitted, that
> > copy will be eliminated. Either way, things work. You can make them work
> > potentially faster if you adopt this API.
> > 
> > There is something to be said about clients preferring their own VideoFrame
> > pooling / management system. I guess it will be up to each cast sender app /
> > subsystem to choose what to use.
> 
> You said "if a generic VideoFrame is submitted..." but how does it know if it
is
> generic or not?

This is specific to each encoder implementation. In the case of VideoToolbox,
the API to encode a frame takes a CVPixelBuffer. It will use private API to
determine if that pixel buffer's backing store was allocated by its internal
pixel buffer pool allocator, which uses more private API to create buffers
residing in memory shared by the app. an encoding daemon and the hardware
encoder.

I'm sorry if I chose my words poorly. The essential goal of this patch is to
allow encoders to allocate frames in an opaque (to chrome and the cast
implementation) fashion.

[jfroy, 2014-11-17 21:23:06]

On 2014/11/17 20:23:14, Alpha wrote:
> I see some issues related to threading. I think it is dangerous to create a
> VideoFrameFactory on cast main thread; use it on any thread; and it
communicates
> with encoder on encoder thread to create most optimized VideoFrame. It might
> work for OSX but might not scale to other implementations. All this threading
> logic should be hidden inside the implementation of VideoEncoder.

I commented on this in more details in my inline comments, but essentially the
design assumes that there will be no interaction between the frame allocator and
the encoder past initialization. I wanted a design that allowed a rendering
thread to allocate frames without enqueuing work for another thread and waiting
for a callback. IN other words, a design that would minimize latency.

> 
> Second issue is how does the client of CastSender knows when to use this API?
In
> Chrome there's certainly no need to call CreateFrame() and it creates
confusion.

I don't disagree with that. It is an entirely opt-in interface. Sometimes it may
be convenient to a particular sender, sometimes it may not. I like your idea of
indicating if the interface might provide some tangible benefits.

> 
> What about using this interface?
> 
> class VideoSender {
> 
>   // Returns true if an optimized version of VideoFrame is
>   // provided. Client of this API should call
>   // CreateOptimizedVideoFrame() to create such VideoFrames
>   // and submit them for encoding.
>   bool ShouldUseOptimizedVideoFrame();
> 
>   // Create an optimized version of VideoFrame asynchronously.
>   void CreateOptimizedVideoFrame(
>     base::TimeDelta timestamp,
>     const base::Callback<media::VideoFrame>& video_frame_cb 
>   );
> };

As I wrote above, I like the idea of indicating to clients that the encoder they
ended up getting offers "fast path" video frame allocation.

But, it is problematic to have this on VideoSender since that object is never
exposed to clients. Presumably all they ever see is the CastSender object and
the audio / video input objects. Or did I misunderstand the design of the cast
implementation (i.e. that FrameSender, VideoSender and AudioSender are all
private objects not to be exposed to the client of the cast implementation).

The other problem is with latency / performance. Relying on two async callbacks,
one to submit the frame creation request and one to deliver the frame, is not
awesome for what is essentially expected to be an independent memory allocation
operation. Considering that the main cast thread can be quite busy with
networking (nearly half a core out of 2 on an iOS device), the extra latency
won't be negligible. I wrote in more detail about my design rationale in my
inline comments.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h...
media/cast/cast_sender.h:42: virtual scoped_refptr<VideoFrame>
CreateFrame(base::TimeDelta timestamp) = 0;
On 2014/11/17 20:23:14, Alpha wrote:
> What if CastSender is not yet initialized? Then there won't be an encoder and
> this method wouldn't know what kind of frames to create. Code change is not
> needed, I think you just need to clarify in comments.

OK, I'll clarify that if the initialization status is not video initialized,
you'll get back null. I thought I didn't need to clarify because if video
initialization fails, CastSender does not create a VideoFrameInput object and
thus you won't ever be able to call that method.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:38: return
video_frame_factory_->CreateFrame(timestamp);
On 2014/11/17 20:23:14, Alpha wrote:
> Why not create VideoFrame through VideoSender?

My understanding is that VideoFrameInput is ref countable to allow it to be used
from any thread and have a lifetime independent from the rest of the cast
instance it belongs to. I wanted to allow clients to create frames from an
arbitrary thread (say a rendering thread) to keep latency in the system to a
minimum. Therefore, I had to have an intermediate ref counted object (the video
frame factory) for the same reason.

VideoSender supports weak pointers for the purpose of allowing the scheme where
some arbitrary thread submits a callback to the main cast thread, where the weak
pointer can be safely checked for validity before proceeding. I certainly could
have followed this model, but I wanted something that could provide better
performance.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:49: scoped_refptr<VideoFrameFactory>
video_frame_factory_;
On 2014/11/17 20:23:14, Alpha wrote:
> Please don't save VideoFrameFactory. It's an internal member of VideoSender.
You
> can already access VidoeFrameFactory through VideoSender or VideoSender can
> create a VideoFrame using internal objects.

See my note above. I made VideoFrameFactory a thread safe ref-counted object for
the explicit purpose of being able to use it outside of the 3 dedicated cast
threads for performance / latency reasons.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder.h:49: virtual scoped_refptr<VideoFrameFactory>
GetVideoFrameFactory() = 0;
On 2014/11/17 20:23:14, Alpha wrote:
> I don't think the threading model for this method is good.
> 
> According to this interface. VideoframeFactory is going to be called on the
main
> thread, while encoder is running on the encoder thread. Which means
> VideoFrameFactory will have to perform cross thread communication with encoder
> implementation. It is easy to make mistakes if each encoder &
VideoFrameFactory
> has to implement this. And it violates the design that VideoEncoder is
> sinlge-threaded.

That's not quite the design I had in mind.

The expectation is that GetVideoFrameFactory() will always be called on the main
cast thread, in keeping with the design of VideoEncoder. Its purpose is to
create a or return an existing video frame factory (essentially a specialized
allocator) that is assumed to be usable from any thread (but perhaps not
concurrently; that detail is up to the implementation). This is why the video
frame factory is a thread-safe ref-counted object.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory.h:20: class VideoFrameFactory : public
base::RefCountedThreadSafe<VideoFrameFactory> {
On 2014/11/17 20:23:14, Alpha wrote:
> I think it's unrealistic that this class is not thread safe, but can be used
on
> any thread and communicates with encoder to creates the most performant
> VideoFrame. This just means internally it has some cross thread communication
> with the encoder or encoder thread.

Not at all. The default implementation I wrote as well as the specialized
implementation for the VideoToolbox encoder do not interact with the encoder in
any way past the point of creation. This is exactly why this is a separate
object as opposed to a method on the VIdeoEncoder. It is expected to be fully
independent.

My comment is poorly worded I think. I meant to say that video frame factories
may not be concurrent (entry my multiple threads at the same time). They should
not be pinned to any particular thread. I make the recommendation that
synchronization should be done by the client / user for performance reasons,
since in many cases a factory will be used either by a single rendering thread
or by a rendering callback which may jump between threads (in a thread pool job
queue system, say) but never run concurrently with a prior invocation of itself.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory_pool_impl.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:18: // Implementation of the
VideoFrameFactory interface using |VideoFramePool|.
On 2014/11/17 20:23:14, Alpha wrote:
> I see no user of this class other than unit test. Do we really need this? Are
we
> going to use it?

I wanted this code path to always be usable, so I provided a reasonable default.
It is an opt-in interface. I haven't looked at leveraging it in other areas in
Chrome, but perhaps a good candidate would be for tab mirroring on OS X where
hardware encoding could be leverage to reduce CPU load and power usage on
laptops?

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:32: VideoFramePool pool_;
On 2014/11/17 20:23:14, Alpha wrote:
> There's really not need for a software based VideoFrame pool. If it is unit
test
> only it should only be in the test.

I think I answer that in my comment above.

[Alpha Left Google, 2014-11-18 02:57:58]

One more question: are you going to implement a new VideoEncoder?

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender.h...
media/cast/cast_sender.h:42: virtual scoped_refptr<VideoFrame>
CreateFrame(base::TimeDelta timestamp) = 0;
On 2014/11/17 21:23:05, jfroy wrote:
> On 2014/11/17 20:23:14, Alpha wrote:
> > What if CastSender is not yet initialized? Then there won't be an encoder
and
> > this method wouldn't know what kind of frames to create. Code change is not
> > needed, I think you just need to clarify in comments.
> 
> OK, I'll clarify that if the initialization status is not video initialized,
> you'll get back null. I thought I didn't need to clarify because if video
> initialization fails, CastSender does not create a VideoFrameInput object and
> thus you won't ever be able to call that method.

You're right. VideoFrameInput is not available until video sending is
initialized.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:38: return
video_frame_factory_->CreateFrame(timestamp);
On 2014/11/17 21:23:06, jfroy wrote:
> On 2014/11/17 20:23:14, Alpha wrote:
> > Why not create VideoFrame through VideoSender?
> 
> My understanding is that VideoFrameInput is ref countable to allow it to be
used
> from any thread and have a lifetime independent from the rest of the cast
> instance it belongs to. I wanted to allow clients to create frames from an
> arbitrary thread (say a rendering thread) to keep latency in the system to a
> minimum. Therefore, I had to have an intermediate ref counted object (the
video
> frame factory) for the same reason.
> 
> VideoSender supports weak pointers for the purpose of allowing the scheme
where
> some arbitrary thread submits a callback to the main cast thread, where the
weak
> pointer can be safely checked for validity before proceeding. I certainly
could
> have followed this model, but I wanted something that could provide better
> performance.

In a latter comment you mentioned that the created VidoeFrameFactory will be
completely isolated from VidoeEncoder after creation.

Consider that VideoFrameFactory is enclosed in VideoFrameInput and
VideoFrameInput is already refcounted thread safe. What about removing
RefCountedThreadSafe from the definition of VideoFrameFactory? Then
VideoFrameInput would just keep it through a scoped_ptr<>.

This would satisfy the need to create VideoFrame on any thread (through
VideoFrameInput). I think scoped_refptr<> for VideoFrameFactory doesn't add much
value.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder.h:49: virtual scoped_refptr<VideoFrameFactory>
GetVideoFrameFactory() = 0;
On 2014/11/17 21:23:06, jfroy wrote:
> On 2014/11/17 20:23:14, Alpha wrote:
> > I don't think the threading model for this method is good.
> > 
> > According to this interface. VideoframeFactory is going to be called on the
> main
> > thread, while encoder is running on the encoder thread. Which means
> > VideoFrameFactory will have to perform cross thread communication with
encoder
> > implementation. It is easy to make mistakes if each encoder &
> VideoFrameFactory
> > has to implement this. And it violates the design that VideoEncoder is
> > sinlge-threaded.
> 
> That's not quite the design I had in mind.
> 
> The expectation is that GetVideoFrameFactory() will always be called on the
main
> cast thread, in keeping with the design of VideoEncoder. Its purpose is to
> create a or return an existing video frame factory (essentially a specialized
> allocator) that is assumed to be usable from any thread (but perhaps not
> concurrently; that detail is up to the implementation). This is why the video
> frame factory is a thread-safe ref-counted object.

In this patch VideoFrameFactory is always accessed through VideoFrameInput which
is already a refcounted thread safe. Making VideoFrameFactory doesn't add more
value. Also there's only one owner of VideoFrameFactory in this patch, what
about change this to:
virtual scoped_ptr<VidoeFrameFactory> CreateVideoFrameFactory() = 0;

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory.h:20: class VideoFrameFactory : public
base::RefCountedThreadSafe<VideoFrameFactory> {
On 2014/11/17 21:23:06, jfroy wrote:
> On 2014/11/17 20:23:14, Alpha wrote:
> > I think it's unrealistic that this class is not thread safe, but can be used
> on
> > any thread and communicates with encoder to creates the most performant
> > VideoFrame. This just means internally it has some cross thread
communication
> > with the encoder or encoder thread.
> 
> Not at all. The default implementation I wrote as well as the specialized
> implementation for the VideoToolbox encoder do not interact with the encoder
in
> any way past the point of creation. This is exactly why this is a separate
> object as opposed to a method on the VIdeoEncoder. It is expected to be fully
> independent.
> 
> My comment is poorly worded I think. I meant to say that video frame factories
> may not be concurrent (entry my multiple threads at the same time). They
should
> not be pinned to any particular thread. I make the recommendation that
> synchronization should be done by the client / user for performance reasons,
> since in many cases a factory will be used either by a single rendering thread
> or by a rendering callback which may jump between threads (in a thread pool
job
> queue system, say) but never run concurrently with a prior invocation of
itself.

This default implementation and the one you mentioned both don't interact with
the encoder pass creation. If this is the case then the design can be much
simpler: there's no need for VideoFrameFactory to be RefCountedThreadSafe.
VideoEncoder just needs to provide a factory method for creating a specific
implementation of VideoFrameFactory. What do you think?

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory_pool_impl.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:18: // Implementation of the
VideoFrameFactory interface using |VideoFramePool|.
On 2014/11/17 21:23:06, jfroy wrote:
> On 2014/11/17 20:23:14, Alpha wrote:
> > I see no user of this class other than unit test. Do we really need this?
Are
> we
> > going to use it?
> 
> I wanted this code path to always be usable, so I provided a reasonable
default.
> It is an opt-in interface. I haven't looked at leveraging it in other areas in
> Chrome, but perhaps a good candidate would be for tab mirroring on OS X where
> hardware encoding could be leverage to reduce CPU load and power usage on
> laptops?

On OSX we have no plan to support H264. Even if hardware encoding is used on OSX
the frame comes from a browser process, there's no creation of VideoFrame in the
renderer (owner of CastSender). On supported platforms we expect the VideoFrame
passed into VideoFrameInput is already optimized for encoding. I see the need to
create a VideoFrame via VidoeFrameInput a special case. I suggest you keep the
interface but move the implementation of this to the unit test.

[jfroy, 2014-11-18 18:03:59]

On 2014/11/18 02:57:58, Alpha wrote:
> One more question: are you going to implement a new VideoEncoder?

I have, it's been in review for a while now (miu has been the principal reviewer
and he's helped me make it much better over the patch sets). See
https://codereview.chromium.org/450693006/.

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:38: return
video_frame_factory_->CreateFrame(timestamp);
On 2014/11/18 02:57:58, Alpha wrote:
> On 2014/11/17 21:23:06, jfroy wrote:
> > On 2014/11/17 20:23:14, Alpha wrote:
> > > Why not create VideoFrame through VideoSender?
> > 
> > My understanding is that VideoFrameInput is ref countable to allow it to be
> used
> > from any thread and have a lifetime independent from the rest of the cast
> > instance it belongs to. I wanted to allow clients to create frames from an
> > arbitrary thread (say a rendering thread) to keep latency in the system to a
> > minimum. Therefore, I had to have an intermediate ref counted object (the
> video
> > frame factory) for the same reason.
> > 
> > VideoSender supports weak pointers for the purpose of allowing the scheme
> where
> > some arbitrary thread submits a callback to the main cast thread, where the
> weak
> > pointer can be safely checked for validity before proceeding. I certainly
> could
> > have followed this model, but I wanted something that could provide better
> > performance.
> 
> In a latter comment you mentioned that the created VidoeFrameFactory will be
> completely isolated from VidoeEncoder after creation.
> 
> Consider that VideoFrameFactory is enclosed in VideoFrameInput and
> VideoFrameInput is already refcounted thread safe. What about removing
> RefCountedThreadSafe from the definition of VideoFrameFactory? Then
> VideoFrameInput would just keep it through a scoped_ptr<>.
> 
> This would satisfy the need to create VideoFrame on any thread (through
> VideoFrameInput). I think scoped_refptr<> for VideoFrameFactory doesn't add
much
> value.

That sounds pretty good.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder.h:49: virtual scoped_refptr<VideoFrameFactory>
GetVideoFrameFactory() = 0;
On 2014/11/18 02:57:58, Alpha wrote:
> On 2014/11/17 21:23:06, jfroy wrote:
> > On 2014/11/17 20:23:14, Alpha wrote:
> > > I don't think the threading model for this method is good.
> > > 
> > > According to this interface. VideoframeFactory is going to be called on
the
> > main
> > > thread, while encoder is running on the encoder thread. Which means
> > > VideoFrameFactory will have to perform cross thread communication with
> encoder
> > > implementation. It is easy to make mistakes if each encoder &
> > VideoFrameFactory
> > > has to implement this. And it violates the design that VideoEncoder is
> > > sinlge-threaded.
> > 
> > That's not quite the design I had in mind.
> > 
> > The expectation is that GetVideoFrameFactory() will always be called on the
> main
> > cast thread, in keeping with the design of VideoEncoder. Its purpose is to
> > create a or return an existing video frame factory (essentially a
specialized
> > allocator) that is assumed to be usable from any thread (but perhaps not
> > concurrently; that detail is up to the implementation). This is why the
video
> > frame factory is a thread-safe ref-counted object.
> 
> In this patch VideoFrameFactory is always accessed through VideoFrameInput
which
> is already a refcounted thread safe. Making VideoFrameFactory doesn't add more
> value. Also there's only one owner of VideoFrameFactory in this patch, what
> about change this to:
> virtual scoped_ptr<VidoeFrameFactory> CreateVideoFrameFactory() = 0;

Sounds good.

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory.h:20: class VideoFrameFactory : public
base::RefCountedThreadSafe<VideoFrameFactory> {
On 2014/11/18 02:57:58, Alpha wrote:
> On 2014/11/17 21:23:06, jfroy wrote:
> > On 2014/11/17 20:23:14, Alpha wrote:
> > > I think it's unrealistic that this class is not thread safe, but can be
used
> > on
> > > any thread and communicates with encoder to creates the most performant
> > > VideoFrame. This just means internally it has some cross thread
> communication
> > > with the encoder or encoder thread.
> > 
> > Not at all. The default implementation I wrote as well as the specialized
> > implementation for the VideoToolbox encoder do not interact with the encoder
> in
> > any way past the point of creation. This is exactly why this is a separate
> > object as opposed to a method on the VIdeoEncoder. It is expected to be
fully
> > independent.
> > 
> > My comment is poorly worded I think. I meant to say that video frame
factories
> > may not be concurrent (entry my multiple threads at the same time). They
> should
> > not be pinned to any particular thread. I make the recommendation that
> > synchronization should be done by the client / user for performance reasons,
> > since in many cases a factory will be used either by a single rendering
thread
> > or by a rendering callback which may jump between threads (in a thread pool
> job
> > queue system, say) but never run concurrently with a prior invocation of
> itself.
> 
> This default implementation and the one you mentioned both don't interact with
> the encoder pass creation. If this is the case then the design can be much
> simpler: there's no need for VideoFrameFactory to be RefCountedThreadSafe.
> VideoEncoder just needs to provide a factory method for creating a specific
> implementation of VideoFrameFactory. What do you think?

Sounds good!

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_frame_factory_pool_impl.h (right):

https://codereview.chromium.org/688423003/diff/60001/media/cast/sender/video_...
media/cast/sender/video_frame_factory_pool_impl.h:18: // Implementation of the
VideoFrameFactory interface using |VideoFramePool|.
On 2014/11/18 02:57:58, Alpha wrote:
> On 2014/11/17 21:23:06, jfroy wrote:
> > On 2014/11/17 20:23:14, Alpha wrote:
> > > I see no user of this class other than unit test. Do we really need this?
> Are
> > we
> > > going to use it?
> > 
> > I wanted this code path to always be usable, so I provided a reasonable
> default.
> > It is an opt-in interface. I haven't looked at leveraging it in other areas
in
> > Chrome, but perhaps a good candidate would be for tab mirroring on OS X
where
> > hardware encoding could be leverage to reduce CPU load and power usage on
> > laptops?
> 
> On OSX we have no plan to support H264. Even if hardware encoding is used on
OSX
> the frame comes from a browser process, there's no creation of VideoFrame in
the
> renderer (owner of CastSender). On supported platforms we expect the
VideoFrame
> passed into VideoFrameInput is already optimized for encoding. I see the need
to
> create a VideoFrame via VidoeFrameInput a special case. I suggest you keep the
> interface but move the implementation of this to the unit test.

Using hardware encoding where available seems like a good idea, but that's way
off topic. Sorry for bringing it up.

What should the interface do for other encoders? Return null? If so, do we still
need another method to query "supports specialized frame allocation", or is a
null return value sufficient?

[jfroy, 2014-11-18 22:48:52]

I uploaded a reworked implementation based on review feedback. The factory is
not ref counted anymore and owned by the VideoFrameInput. I deleted the
VideoFramePool implementation; encoders that don't support affinity just return
null. I simplified the unit tests to just check that null is returned.

My VideoToolbox encoder will include unit tests for its specialization of the
VideoFrameFactory interface. I didn't think there was a point in unit testing a
default implementation that won't be shipped or used by anyone.

[Alpha Left Google, 2014-11-19 19:22:19]

https://codereview.chromium.org/688423003/diff/80001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/688423003/diff/80001/media/cast/cast_sender.h...
media/cast/cast_sender.h:42: virtual scoped_refptr<VideoFrame>
CreateFrame(base::TimeDelta timestamp) = 0;
This looks good. What about adding the method bool ShouldCreateOptimizedFrame()
const? It can return true only if VideoFrameFactory is available. This way it is
clear that client should use this method to create or not.

[jfroy, 2014-11-19 21:56:49]

I added bool SupportsCreateOptimizedFrame() and renamed CreateFrame() to
CreateOptimizedFrame(). I also clarified the documentation some more.

[Alpha Left Google, 2014-11-19 23:20:21]

lgtm

[jfroy, 2014-11-19 23:20:53]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2014-11-19 23:22:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688423003/100001

[miu, 2014-11-19 23:24:47]

One thing I don't quite understand: Typically, the media::VideoFrame instances
passed to Cast are wrappers around shared memory (i.e., a video frame that is
generated by the browser process, and only read from the renderer process).  So,
with this change, are you proposing making a memcpy() from that video frame into
the "optimized" VideoFrame before passing it to Cast?  If so, does the CPU time
spent on the memcpy() outweigh the CPU savings spent on the optimized encode?

https://codereview.chromium.org/688423003/diff/100001/media/cast/sender/video...
File media/cast/sender/video_encoder.h (right):

https://codereview.chromium.org/688423003/diff/100001/media/cast/sender/video...
media/cast/sender/video_encoder.h:50: virtual scoped_ptr<VideoFrameFactory>
CreateVideoFrameFactory() = 0;
Let's just define this method to return nullptr here.  Then, it wouldn't be
necessary to override and implement it in all the subclasses.

[jfroy, 2014-11-19 23:29:09]

The CQ bit was unchecked by jfroy@chromium.org

[jfroy, 2014-11-19 23:49:10]

On 2014/11/19 23:24:47, miu wrote:
> One thing I don't quite understand: Typically, the media::VideoFrame instances
> passed to Cast are wrappers around shared memory (i.e., a video frame that is
> generated by the browser process, and only read from the renderer process). 
So,
> with this change, are you proposing making a memcpy() from that video frame
into
> the "optimized" VideoFrame before passing it to Cast?  If so, does the CPU
time
> spent on the memcpy() outweigh the CPU savings spent on the optimized encode?

In this case, I imagine it's going to be a wash. For example, in the case of the
VideoToolbox encoder, you will either copy into an optimized VideoFrame, or
VideoToolbox will internally copy into a buffer it will allocate from its
internal buffer pool.

You're only going to see a performance increase if you generate the content
directly into an optimized VideoFrame. This is my ultimate use case.

>
https://codereview.chromium.org/688423003/diff/100001/media/cast/sender/video...
> File media/cast/sender/video_encoder.h (right):
> 
>
https://codereview.chromium.org/688423003/diff/100001/media/cast/sender/video...
> media/cast/sender/video_encoder.h:50: virtual scoped_ptr<VideoFrameFactory>
> CreateVideoFrameFactory() = 0;
> Let's just define this method to return nullptr here.  Then, it wouldn't be
> necessary to override and implement it in all the subclasses.

There aren't many VideoEncoder implementations, so I'm not sure if it's better
to keep VideoEncoder a pure interface or to give it an implementation for the
sole purpose of providing a default for that method. I'm happy to make the
change if you think that's a net win.

[miu, 2014-11-20 00:00:25]

lgtm

On 2014/11/19 23:49:10, jfroy wrote:
> On 2014/11/19 23:24:47, miu wrote:
> > does the CPU time
> > spent on the memcpy() outweigh the CPU savings spent on the optimized
encode?
> 
> will internally copy into a buffer it will allocate from its
> internal buffer pool.

Ah, okay.  No problem here then.
 
> > media/cast/sender/video_encoder.h:50: virtual scoped_ptr<VideoFrameFactory>
> > CreateVideoFrameFactory() = 0;
> > Let's just define this method to return nullptr here.  Then, it wouldn't be
> > necessary to override and implement it in all the subclasses.
> 
> There aren't many VideoEncoder implementations, so I'm not sure if it's better
> to keep VideoEncoder a pure interface or to give it an implementation for the
> sole purpose of providing a default for that method. I'm happy to make the
> change if you think that's a net win.

Since VideoEncoder is a base class only used in this dir in the code base, I
would lean towards it being an abstract base class rather than a pure interface
class.  Less code that does more is my general preference.  ;-)

[jfroy, 2014-11-20 00:10:18]

On 2014/11/20 00:00:25, miu wrote:
> lgtm
> 
> On 2014/11/19 23:49:10, jfroy wrote:
> > On 2014/11/19 23:24:47, miu wrote:
> > > does the CPU time
> > > spent on the memcpy() outweigh the CPU savings spent on the optimized
> encode?
> > 
> > will internally copy into a buffer it will allocate from its
> > internal buffer pool.
> 
> Ah, okay.  No problem here then.
>  
> > > media/cast/sender/video_encoder.h:50: virtual
scoped_ptr<VideoFrameFactory>
> > > CreateVideoFrameFactory() = 0;
> > > Let's just define this method to return nullptr here.  Then, it wouldn't
be
> > > necessary to override and implement it in all the subclasses.
> > 
> > There aren't many VideoEncoder implementations, so I'm not sure if it's
better
> > to keep VideoEncoder a pure interface or to give it an implementation for
the
> > sole purpose of providing a default for that method. I'm happy to make the
> > change if you think that's a net win.
> 
> Since VideoEncoder is a base class only used in this dir in the code base, I
> would lean towards it being an abstract base class rather than a pure
interface
> class.  Less code that does more is my general preference.  ;-)

sgtm. I'll make the change.

[jfroy, 2014-11-20 02:16:46]

Default impl of CreateVideoFrameFactory in VideoEncoder.

[jfroy, 2014-11-20 02:24:38]

Final patch cleanup.

[jfroy, 2014-11-20 02:26:53]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2014-11-20 02:27:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688423003/140001

[commit-bot: I haz the power, 2014-11-20 04:31:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-20 04:32:24]

Patchset 8 (id:??) landed as
https://crrev.com/dcff178ab70511a4135895bc092ad3b871a0ae3d
Cr-Commit-Position: refs/heads/master@{#304973}