Add a VideoDecodeContext that provides resources for a VideoDecodeEngine

Add a VideoDecodeContext that provides resources for a VideoDecodeEngine

Also define a Gles2VideoDecodeContext to be used in the chrome renderer.

BUG=53714
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58772

[scherkus (not reviewing), 2010-08-30 19:29:52]

http://codereview.chromium.org/3233003/diff/8001/9003
File chrome/renderer/media/gles2_video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/8001/9003#newcode33
chrome/renderer/media/gles2_video_decode_context.h:33: //    software video
decoding and use them  as if they are allocated in plain
nit: remove extra space between "them" and "as"

http://codereview.chromium.org/3233003/diff/8001/9003#newcode35
chrome/renderer/media/gles2_video_decode_context.h:35: //    bwith the GPU
process). An additional step of uploading the content to
bwith?

http://codereview.chromium.org/3233003/diff/8001/9003#newcode36
chrome/renderer/media/gles2_video_decode_context.h:36: //    video memory is
needed. Since VideoDecodeEngien is unaware of the video
VideoDecodeEngien -> VideoDecodeEngine

http://codereview.chromium.org/3233003/diff/8001/9003#newcode60
chrome/renderer/media/gles2_video_decode_context.h:60: // as regular texturs.
texturs -> textures

http://codereview.chromium.org/3233003/diff/8001/9003#newcode64
chrome/renderer/media/gles2_video_decode_context.h:64: // This class is accessed
on two thread, namely the Render Thread and the
thread -> threads

http://codereview.chromium.org/3233003/diff/8001/9003#newcode89
chrome/renderer/media/gles2_video_decode_context.h:89: // TODO(hclam): Neede to
figure out which thread to destroy this object.
Neede -> Need
to destroy -> destroys

http://codereview.chromium.org/3233003/diff/8001/9004
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3233003/diff/8001/9004#newcode2423
chrome/renderer/render_view.cc:2423: // the Gles2VideoDecodeContext.
oh boy we really need to fix that factory stuff AND QUICK! :P

http://codereview.chromium.org/3233003/diff/8001/9005
File media/video/video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/8001/9005#newcode28
media/video/video_decode_context.h:28: // If a hardware device is not need this
method should return NULL.
need -> needed

http://codereview.chromium.org/3233003/diff/8001/9005#newcode33
media/video/video_decode_context.h:33: virtual void ReleaseDevice(void* device);
so does GetDevice effectively add a ref to the device?

http://codereview.chromium.org/3233003/diff/8001/9005#newcode43
media/video/video_decode_context.h:43: virtual void ReleaseVideoFrames() = 0;
would we ever release but not destroy?

I'm wondering if it's better to make allocate an "initialize" and then have a
destroy for teardown

that way we could also make initialize callback be 0-param then have frame
accessor methods on the context that the user can call

[Alpha Left Google, 2010-08-31 00:07:23]

http://codereview.chromium.org/3233003/diff/8001/9003
File chrome/renderer/media/gles2_video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/8001/9003#newcode33
chrome/renderer/media/gles2_video_decode_context.h:33: //    software video
decoding and use them  as if they are allocated in plain
On 2010/08/30 19:29:52, scherkus wrote:
> nit: remove extra space between "them" and "as"

Done.

http://codereview.chromium.org/3233003/diff/8001/9003#newcode35
chrome/renderer/media/gles2_video_decode_context.h:35: //    bwith the GPU
process). An additional step of uploading the content to
On 2010/08/30 19:29:52, scherkus wrote:
> bwith?

Done.

http://codereview.chromium.org/3233003/diff/8001/9003#newcode36
chrome/renderer/media/gles2_video_decode_context.h:36: //    video memory is
needed. Since VideoDecodeEngien is unaware of the video
On 2010/08/30 19:29:52, scherkus wrote:
> VideoDecodeEngien -> VideoDecodeEngine

Done.

http://codereview.chromium.org/3233003/diff/8001/9003#newcode60
chrome/renderer/media/gles2_video_decode_context.h:60: // as regular texturs.
On 2010/08/30 19:29:52, scherkus wrote:
> texturs -> textures

Done.

http://codereview.chromium.org/3233003/diff/8001/9003#newcode64
chrome/renderer/media/gles2_video_decode_context.h:64: // This class is accessed
on two thread, namely the Render Thread and the
On 2010/08/30 19:29:52, scherkus wrote:
> thread -> threads

Done.

http://codereview.chromium.org/3233003/diff/8001/9003#newcode89
chrome/renderer/media/gles2_video_decode_context.h:89: // TODO(hclam): Neede to
figure out which thread to destroy this object.
On 2010/08/30 19:29:52, scherkus wrote:
> Neede -> Need
> to destroy -> destroys

Done.

http://codereview.chromium.org/3233003/diff/8001/9005
File media/video/video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/8001/9005#newcode28
media/video/video_decode_context.h:28: // If a hardware device is not need this
method should return NULL.
On 2010/08/30 19:29:52, scherkus wrote:
> need -> needed

Done.

http://codereview.chromium.org/3233003/diff/8001/9005#newcode33
media/video/video_decode_context.h:33: virtual void ReleaseDevice(void* device);
On 2010/08/30 19:29:52, scherkus wrote:
> so does GetDevice effectively add a ref to the device?

That's right. Some device can be addrefed like ID3DDevice but maybe not for
others, so to unify that GetDevice() always add a ref.

http://codereview.chromium.org/3233003/diff/8001/9005#newcode43
media/video/video_decode_context.h:43: virtual void ReleaseVideoFrames() = 0;
On 2010/08/30 19:29:52, scherkus wrote:
> would we ever release but not destroy?
> 
> I'm wondering if it's better to make allocate an "initialize" and then have a
> destroy for teardown
> 
> that way we could also make initialize callback be 0-param then have frame
> accessor methods on the context that the user can call
> 
> 

We can't guarantee that video frames are always the same size. We still need
allocate / release pair to handle format change.

[scherkus (not reviewing), 2010-08-31 02:58:56]

http://codereview.chromium.org/3233003/diff/15001/16005
File media/video/video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/15001/16005#newcode33
media/video/video_decode_context.h:33: virtual void ReleaseDevice(void* device);
another question: when/why would we release a device?

the format change part for release video frames I understand.. but it seems that
the device is already "reffed" because it's held onto the context, which in turn
exists until we call Destroy()

thoughts?

http://codereview.chromium.org/3233003/diff/15001/16005#newcode38
media/video/video_decode_context.h:38: AllocationCompleteCallback* callback) =
0;
Few more considerations:
  - would we ever want to hold onto one frame while releasing the others?
  - can we avoid a 2-param callback (int, VideoFrame**> and instead query the
context for the frames?
  - how do we know if allocation succeeded?
  - don't we want to specify the format or something?

I guess I'm a bit confused from just looking at this CL how a decoder (say even
the FFmpeg software decoder) would use this context.  What if FFmpeg only calls
AllocateVideoFrame()/ReleaseVideoFrame() one frame at a time as needed?

[vrk (LEFT CHROMIUM), 2010-08-31 19:42:43]

http://codereview.chromium.org/3233003/diff/15001/16005
File media/video/video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/15001/16005#newcode38
media/video/video_decode_context.h:38: AllocationCompleteCallback* callback) =
0;
On 2010/08/31 02:58:56, scherkus wrote:
> Few more considerations:
>   - would we ever want to hold onto one frame while releasing the others?
>   - can we avoid a 2-param callback (int, VideoFrame**> and instead query the
> context for the frames?
>   - how do we know if allocation succeeded?
>   - don't we want to specify the format or something?
> 
> I guess I'm a bit confused from just looking at this CL how a decoder (say
even
> the FFmpeg software decoder) would use this context.  What if FFmpeg only
calls
> AllocateVideoFrame()/ReleaseVideoFrame() one frame at a time as needed?

In this design, I think the decoder would maintain something like a pool of
texture-backed video frames, and it would reuse frames from the pool as needed.
So it would allocate |pool| frames using the decode context somewhere like in
the initialize function, then release/reallocate the entire pool of frames as
necessary when the format changes (or the decode is completely done). 

I like the idea of having an AllocateVideoFrame()/ReleaseVideoFrame() paradigm
where FFmpeg asks the VideoDecoderContext for one frame at a time as needed. I
think it'd be easy to inject into the existing decoder code, and we would also
be able to use the glMapTexSubImage2D/glUnmapTexSubImage2D as is. (If we wanted
to maintain a queue of texture-backed frames, we would have to add gl commands
to signal a write to screen or something since the current implementation has
the screen updated on glUnmap.) Plus the VideoRenderer maintains a queue of
decoded VideoFrames anyway.

But Alpha said there were some performance issues with thread-switching and
stuff... I don't have a good sense of the trade-offs for these issues. Maybe
Alpha can follow up with more details.

[vrk (LEFT CHROMIUM), 2010-08-31 23:22:21]

> But Alpha said there were some performance issues with thread-switching and
> stuff... I don't have a good sense of the trade-offs for these issues. Maybe
> Alpha can follow up with more details.

OK, I understand things better! Alpha explained the renderer/decoder
thread-switching issues to me in person. (Also, sorry if I was explaining the
obvious, Andrew! I'm never sure how much is clear/unclear since stuff is always
unclear to me, haha.)

I didn't realize that the current plan was to move the frame pool/frame
management completely into the decoder and getting rid of the frames queue from
the video renderer. I totally agree that having the decoder *and* the renderer
manage frames is a bad idea. But my thought was to move the buffer/frame pool
management into the render completely instead of in the decoder, such that there
wouldn't have to be a thread switch between the decoder and renderer threads to
issue glMap/UnmapTexSubImage2D calls. However Alpha said that in most other
situations *except* for this particular situation (such as seek/pause), it is
much easier to have the decoder manage frames instead of the renderer...

So yeah, in light of that, I think that having the proposed design could work -
though there is still the issue with the glMapTexSubImage2D commands not doing
what we want it to do. I can set up time for us to talk with Gregg Tavares about
possibilities for how to get around that.

[scherkus (not reviewing), 2010-09-01 03:22:17]

cool!

let's discuss this tomorrow complete with snacks, beverages and a whiteboard :)

[Alpha Left Google, 2010-09-03 01:33:20]

The API is updated. Please have an other look.

[scherkus (not reviewing), 2010-09-03 17:50:13]

+ajwong for allocate/release comments (I think you had some during that little
whiteboard session)

http://codereview.chromium.org/3233003/diff/26001/27005
File media/video/video_decode_context.h (right):

http://codereview.chromium.org/3233003/diff/26001/27005#newcode39
media/video/video_decode_context.h:39: virtual void ReleaseVideoFrames(int n,
VideoFrame* frames) = 0;
this is still a bit clumsy

so AllocateVideoFrames returns an array of VideoFrame* (basically a
VideoFrame**).  Do I copy the array of pointers or do I now own the array of
pointers?

What am I passing in here?  The pointer to the array returned from
AllocateVideoFrames?  if I copied the array of pointers, is the implementor
looking for and releasing each element, or is the implementor looking for the
just the start of the array (the one passed back from AllocateVideoFrames)?

Also why are we passing in n?  If I allocated 3 frames, what happens when I pass
in 2?

I'm wondering if this would be easier to understand if it worked like calloc():
  - AllocateVideoFrame(n) -> a single VideoFrame* that can be accessed at
indices [0-2]
  - ReleaseVideoFrame(VideoFrame*) -> pass back in original pointer from
AllocateVideoFrame

Also the documentation still hasn't be updated to explain whether we're allow to
call Allocate() as much as we want.  If we are.. then I'm wondering if perhaps
we should stick with just single Allocate/Release.  If you want three frames,
fire off 3 allocs + callbacks and wait for them all to finish.  This might be
over pedantic, but what if we only allocate 2/3 frames -- is it all/nothing? 
Will the implementor have to free whatever it allocated and return failure
somehow?

[scherkus (not reviewing), 2010-09-03 17:52:17]

so while the API feels a bit strange -- rest is LGTM

you can go ahead and land this but do consider the alloc/release interface +
what implementors will be responsible for

[Alpha Left Google, 2010-09-03 18:18:28]

On 2010/09/03 17:50:13, scherkus wrote:
> +ajwong for allocate/release comments (I think you had some during that little
> whiteboard session)
> 
> http://codereview.chromium.org/3233003/diff/26001/27005
> File media/video/video_decode_context.h (right):
> 
> http://codereview.chromium.org/3233003/diff/26001/27005#newcode39
> media/video/video_decode_context.h:39: virtual void ReleaseVideoFrames(int n,
> VideoFrame* frames) = 0;
> this is still a bit clumsy
> 
> so AllocateVideoFrames returns an array of VideoFrame* (basically a
> VideoFrame**).  Do I copy the array of pointers or do I now own the array of
> pointers?
> 
> What am I passing in here?  The pointer to the array returned from
> AllocateVideoFrames?  if I copied the array of pointers, is the implementor
> looking for and releasing each element, or is the implementor looking for the
> just the start of the array (the one passed back from AllocateVideoFrames)?
> 
> Also why are we passing in n?  If I allocated 3 frames, what happens when I
pass
> in 2?
> 
> I'm wondering if this would be easier to understand if it worked like
calloc():
>   - AllocateVideoFrame(n) -> a single VideoFrame* that can be accessed at
> indices [0-2]
>   - ReleaseVideoFrame(VideoFrame*) -> pass back in original pointer from
> AllocateVideoFrame
> 

The callback will return with an array of pointers. The receiver should be
copying the array. I'll add that to the documentation.

Release video frame will pass in an array of pointers.

If we do it just like calloc(), it will be easier in to under but it'll be
harder to use. Imagine a filter just want to allocate 3 buffers it needs to have
logic that receives buffer, post a task to decoder thread, queue them, and then
when the count is right switch to the next state. But if you always allocate in
a batch, which the decode engine will, there's no such logic needed. Yes
allocate can be called multiple times, but that's only during dimension change,
in that case you'll release in a batch, and then allocate in a batch, both
thread switching and logic will be easy. I'd like to avoid having too much in
the decode engine just for this.


> Also the documentation still hasn't be updated to explain whether we're allow
to
> call Allocate() as much as we want.  If we are.. then I'm wondering if perhaps
> we should stick with just single Allocate/Release.  If you want three frames,
> fire off 3 allocs + callbacks and wait for them all to finish.  This might be
> over pedantic, but what if we only allocate 2/3 frames -- is it all/nothing? 
> Will the implementor have to free whatever it allocated and return failure
> somehow?

Since the use case is always like this:

Allocate a batch
Release all
Allocate another batch
Release all

What about we do:
Allocate(n, ...);
ReleaseAll(); ?