Pass Client pointer in Initialize() for VDA/VEA

Pass Client pointer in Initialize() for VDA/VEA

Wait until Initialize() is called on a Video{Decode,Encode}Accelerator to
pass in its Client pointer.  This is done to better separate the construction
and initialization of the VDA/VEA, so its Client does not have to guarantee
its own lifetime until it it is ready to call Initialize() (since the
VDA/VEA expects is Client to be valid until itself is Destroy()ed).

BUG=325984
TEST=local build, run on CrOS snow, build on desktop Linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253728

[sheu, 2014-02-19 00:28:11]

CCing people for information purposes at the moment.  I'm going to make sure
this doesn't break stuff before moving to review.

[sheu, 2014-02-19 21:11:01]

Time to start reviewing.

hshi@: PTAL at media/video/video_encode_accelerator.h and
media/video/video_decode_accelerator.h and see if it fits your needs.  The rest
of the files are pretty much just fallout.

[hshi1, 2014-02-19 21:13:45]

On 2014/02/19 21:11:01, sheu wrote:
> Time to start reviewing.
> 
> hshi@: PTAL at media/video/video_encode_accelerator.h and
> media/video/video_decode_accelerator.h and see if it fits your needs.  The
rest
> of the files are pretty much just fallout.

media/video/video_(en|de)code_accelerator.h LGTM

[sheu, 2014-02-19 23:20:26]

Since fischman@ is out:

scherkus@: PTAL, thanks.  Most of the CL (i.e. all the VDA implementations)
should be functionally no-op.

[scherkus (not reviewing), 2014-02-20 18:52:08]

lgtm w/ nit

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_encode_accelerator.h:113: media::VideoFrame::Format
input_format,
nit: remove media:: ?

[sheu, 2014-02-21 02:03:53]

sievers@: PTAL at content/common/gpu/client/*.  Thanks!

[sheu, 2014-02-24 22:08:31]

sievers@: PTAL at content/common/gpu/client/*.  Thanks!

[Ami GONE FROM CHROMIUM, 2014-02-24 23:12:40]

bunch o' nits.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:23:
GpuVideoDecodeAcceleratorHost::GpuVideoDecodeAcceleratorHost(
NULL-initialize client_ for hygiene.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:73: // Reference
to the client that will receive callbacks from the decoder.
This isn't a "reference", so 
// Client listening to decoder callbacks.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:16:
GpuVideoEncodeAcceleratorHost::GpuVideoEncodeAcceleratorHost(
NULL-init client_

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:67:
AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
NULL-initialize client_.
(gonna stop making this comment now; please make a pass for it through the whole
CL)

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
File content/renderer/media/pepper_platform_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
content/renderer/media/pepper_platform_video_decoder.cc:47: return
decoder_->Initialize(this, profile);
nit: l.44-47 are:
return decoder && decoder_->Initialize(this, profile);

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
File content/renderer/media/rtc_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
content/renderer/media/rtc_video_decoder.cc:677: vda_->Initialize(this,
profile);
CHECK this doesn't fail (b/c gvdah.cc can't)

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/filters/gp...
media/filters/gpu_video_decoder.cc:198: vda_->Initialize(this,
config.profile());
this can return false according to vda.h, but not according to gvdah.cc.  CHECK
it.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_decode_accelerator.h:89: //  be valid for the rest of the
lifetime of this decoder, until Destroy()
I'd drop "for the rest of the lifetime of this decoder, " since the final clause
is more precise and useful.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_decode_accelerator.h:94: virtual bool Initialize(Client*
client, VideoCodecProfile profile) = 0;
In-params usually precede out-params, and POD params are innier than params that
will get methods invoked on them in the future (which seems closer to out-param
than in-param to me) so if it was me I'd make |client| the last param here (and
ditto in vea.h).  But that'd mean touching a lot of files in this CL so I'm not
going to insist...

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_encode_accelerator.h:101: //  be valid for the rest of the
lifetime of this encoder, until Destroy()
ditto drop "for the rest of the lifetime of this encoder, "

[sheu, 2014-02-24 23:48:20]

Updated, PTAL.

Left a comment about the ordering of parameters to Initialize().  Let me know.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:23:
GpuVideoDecodeAcceleratorHost::GpuVideoDecodeAcceleratorHost(
On 2014/02/24 23:12:40, Ami Fischman wrote:
> NULL-initialize client_ for hygiene.

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:73: // Reference
to the client that will receive callbacks from the decoder.
On 2014/02/24 23:12:40, Ami Fischman wrote:
> This isn't a "reference", so 
> // Client listening to decoder callbacks.

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:16:
GpuVideoEncodeAcceleratorHost::GpuVideoEncodeAcceleratorHost(
On 2014/02/24 23:12:40, Ami Fischman wrote:
> NULL-init client_

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:67:
AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
On 2014/02/24 23:12:40, Ami Fischman wrote:
> NULL-initialize client_.
> (gonna stop making this comment now; please make a pass for it through the
whole
> CL)

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
File content/renderer/media/pepper_platform_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
content/renderer/media/pepper_platform_video_decoder.cc:47: return
decoder_->Initialize(this, profile);
On 2014/02/24 23:12:40, Ami Fischman wrote:
> nit: l.44-47 are:
> return decoder && decoder_->Initialize(this, profile);

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
File content/renderer/media/rtc_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/content/renderer...
content/renderer/media/rtc_video_decoder.cc:677: vda_->Initialize(this,
profile);
On 2014/02/24 23:12:40, Ami Fischman wrote:
> CHECK this doesn't fail (b/c gvdah.cc can't)

The call point for CreateVDA() is in RTCVideoDecoder::Create().  Constructor
failure is checked for there; I guess we can fold this in to have a more useful
failure path than just a CHECK().

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/filters/gp...
media/filters/gpu_video_decoder.cc:198: vda_->Initialize(this,
config.profile());
On 2014/02/24 23:12:40, Ami Fischman wrote:
> this can return false according to vda.h, but not according to gvdah.cc. 
CHECK
> it.

I'm going to fold it into the previous check.  I'd like to avoid the implicit
assumption that this is a GVDAH at this layer.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_decode_accelerator.h:89: //  be valid for the rest of the
lifetime of this decoder, until Destroy()
On 2014/02/24 23:12:40, Ami Fischman wrote:
> I'd drop "for the rest of the lifetime of this decoder, " since the final
clause
> is more precise and useful.

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_decode_accelerator.h:94: virtual bool Initialize(Client*
client, VideoCodecProfile profile) = 0;
On 2014/02/24 23:12:40, Ami Fischman wrote:
> In-params usually precede out-params, and POD params are innier than params
that
> will get methods invoked on them in the future (which seems closer to
out-param
> than in-param to me) so if it was me I'd make |client| the last param here
(and
> ditto in vea.h).  But that'd mean touching a lot of files in this CL so I'm
not
> going to insist...

My feeling is that |client| is more innate to the state of the VDA/VEA than
|profile|, since the client is essentially the parent of this whole object.  Yay
feelings.

I'd rather fix it once in this CL than not bother and then have it bother over
and over again people whenever they see the code.  So if you really think the
other ordering is better I'll just make a pass and do it.  Just let me know.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_encode_accelerator.h:101: //  be valid for the rest of the
lifetime of this encoder, until Destroy()
On 2014/02/24 23:12:40, Ami Fischman wrote:
> ditto drop "for the rest of the lifetime of this encoder, "

Done.

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_encode_accelerator.h:113: media::VideoFrame::Format
input_format,
On 2014/02/20 18:52:09, scherkus wrote:
> nit: remove media:: ?

Done.

[Ami GONE FROM CHROMIUM, 2014-02-25 00:01:34]

PS#3 LGTM

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/170843004/diff/250001/media/video/vide...
media/video/video_decode_accelerator.h:94: virtual bool Initialize(Client*
client, VideoCodecProfile profile) = 0;
On 2014/02/24 23:48:20, sheu wrote:
> On 2014/02/24 23:12:40, Ami Fischman wrote:
> > In-params usually precede out-params, and POD params are innier than params
> that
> > will get methods invoked on them in the future (which seems closer to
> out-param
> > than in-param to me) so if it was me I'd make |client| the last param here
> (and
> > ditto in vea.h).  But that'd mean touching a lot of files in this CL so I'm
> not
> > going to insist...
> 
> My feeling is that |client| is more innate to the state of the VDA/VEA than
> |profile|, since the client is essentially the parent of this whole object. 
Yay
> feelings.
> 
> I'd rather fix it once in this CL than not bother and then have it bother over
> and over again people whenever they see the code.  So if you really think the
> other ordering is better I'll just make a pass and do it.  Just let me know.

Yeah, I definitely think profile should come first.

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
media/filters/gpu_video_decoder.cc:194: if (!vda_ || !vda_->Initialize(this,
config.profile())) {
I love this formulation because it crystallizes so well my failure in making the
IPC & VDA interfaces symmetric by highlighting the repetition of the profile
parameter to CreateVDA & Initialize (the second of which is a no-op).
Oh well.

[no sievers, 2014-02-25 00:04:57]

On 2014/02/24 22:08:31, sheu wrote:
> sievers@: PTAL at content/common/gpu/client/*.  Thanks!

lgtm

[sheu, 2014-02-25 00:37:15]

Arguments moved for both VDA and VEA.

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
media/filters/gpu_video_decoder.cc:194: if (!vda_ || !vda_->Initialize(this,
config.profile())) {
On 2014/02/25 00:01:35, Ami Fischman wrote:
> I love this formulation because it crystallizes so well my failure in making
the
> IPC & VDA interfaces symmetric by highlighting the repetition of the profile
> parameter to CreateVDA & Initialize (the second of which is a no-op).
> Oh well.

I don't think that's innately a failure -- it's more an IPC optimization, which
saves a round-trip by collapsing the construction and initialization.  That
optimization just has some uglier externally-visible effects.

[sheu, 2014-02-25 00:41:16]

joi@: PTAL at content/public/renderer/*
yzshen@: PTAL content/renderer/pepper/ppb_video_decoder_impl.cc

Both should be simple.  Thanks!

[Ami GONE FROM CHROMIUM, 2014-02-25 00:50:50]

Still LGTMl; thanks for reordering.

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/170843004/diff/740001/media/filters/gp...
media/filters/gpu_video_decoder.cc:194: if (!vda_ || !vda_->Initialize(this,
config.profile())) {
On 2014/02/25 00:37:16, sheu wrote:
> On 2014/02/25 00:01:35, Ami Fischman wrote:
> > I love this formulation because it crystallizes so well my failure in making
> the
> > IPC & VDA interfaces symmetric by highlighting the repetition of the profile
> > parameter to CreateVDA & Initialize (the second of which is a no-op).
> > Oh well.
> 
> I don't think that's innately a failure -- it's more an IPC optimization,
which
> saves a round-trip by collapsing the construction and initialization.  That
> optimization just has some uglier externally-visible effects.

<MadTV>Lowered expectaaations...</MadTV>

[Jói, 2014-02-25 08:59:50]

//content/public/renderer LGTM

[yzshen1, 2014-02-25 22:05:40]

ppb_video_decoder_impl.cc LGTM

[sheu, 2014-02-26 00:22:31]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:35:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/170843004/800001

[commit-bot: I haz the power, 2014-02-26 00:35:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:35:58]

Failed to apply patch for content/public/renderer/video_encode_accelerator.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/public/renderer/video_encode_accelerator.cc
  Hunk #1 FAILED at 8.
  1 out of 1 hunk FAILED -- saving rejects to file
content/public/renderer/video_encode_accelerator.cc.rej

Patch:       content/public/renderer/video_encode_accelerator.cc
Index: content/public/renderer/video_encode_accelerator.cc
diff --git a/content/public/renderer/video_encode_accelerator.cc
b/content/public/renderer/video_encode_accelerator.cc
index
3ed1875748a17e961ad5fc15570092e5483496e7..3f564f02ca40592606f4ee40537366d7bb69ccd9
100644
--- a/content/public/renderer/video_encode_accelerator.cc
+++ b/content/public/renderer/video_encode_accelerator.cc
@@ -8,14 +8,13 @@
 
 namespace content {
 
-scoped_ptr<media::VideoEncodeAccelerator>
-CreateVideoEncodeAccelerator(media::VideoEncodeAccelerator::Client* client) {
+scoped_ptr<media::VideoEncodeAccelerator> CreateVideoEncodeAccelerator() {
   scoped_ptr<media::VideoEncodeAccelerator> vea;
 
   scoped_refptr<RendererGpuVideoAcceleratorFactories> gpu_factories =
         RenderThreadImpl::current()->GetGpuFactories();
   if (gpu_factories.get())
-    vea = gpu_factories->CreateVideoEncodeAccelerator(client).Pass();
+    vea = gpu_factories->CreateVideoEncodeAccelerator().Pass();
 
   return vea.Pass();
 }

[sheu, 2014-02-26 00:38:25]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/170843004/820001

[commit-bot: I haz the power, 2014-02-26 05:11:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:11:52]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, ipc_tests, jingle_unittests, media_unittests,
mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests,
remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sheu, 2014-02-26 22:12:09]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:13:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/170843004/840001

[commit-bot: I haz the power, 2014-02-27 05:58:53]

Change committed as 253728