Remove WebMediaPlayerProxy::video_decoder_ & FFmpegVideoDecoder::set_decryptor.

Remove WebMediaPlayerProxy::video_decoder_ & FFmpegVideoDecoder::set_decryptor.


BUG=None

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

[acolwell GONE FROM CHROMIUM, 2012-08-06 19:21:39]

While I was trying to move decoder initialization into VideoRendererBase, I
discovered that Remove WebMediaPlayerProxy::video_decoder_ doesn't appear to be
used anywhere and that FFmpegVideoDecoder::set_decryptor() could be removed with
a little bit of test code reorganization.

[xhwang, 2012-08-06 20:05:11]

+scherkus for review.

Thanks for the patch. Passing the decryptor in the constructor of FFVD is one of
the original approaches I tried. It's okay for me. The reason that we chose the
current implementation, IIRC, was that we didn't like to pass NULL's all around
in case the FFVD doesn't need a decryptor (or doesn't support decryption).

For example, this CL will break player_x11, so we need to pass another NULL in
the ctor of FFVD there too.

scherkus, what's your latest opinion on this?

xhwang

[scherkus (not reviewing), 2012-08-06 20:40:27]

On 2012/08/06 20:05:11, xhwang wrote:
> +scherkus for review.
> 
> Thanks for the patch. Passing the decryptor in the constructor of FFVD is one
of
> the original approaches I tried. It's okay for me. The reason that we chose
the
> current implementation, IIRC, was that we didn't like to pass NULL's all
around
> in case the FFVD doesn't need a decryptor (or doesn't support decryption).
> 
> For example, this CL will break player_x11, so we need to pass another NULL in
> the ctor of FFVD there too.
> 
> scherkus, what's your latest opinion on this?

I'm indifferent -- I imagine we'll be shuffling around who owns the decryptor
more in the future. If this gets us closer to cleaning up initialization +
ownership I'm cool with acolwell's CL

[xhwang, 2012-08-06 20:46:20]

On 2012/08/06 20:40:27, scherkus wrote:
> On 2012/08/06 20:05:11, xhwang wrote:
> > +scherkus for review.
> > 
> > Thanks for the patch. Passing the decryptor in the constructor of FFVD is
one
> of
> > the original approaches I tried. It's okay for me. The reason that we chose
> the
> > current implementation, IIRC, was that we didn't like to pass NULL's all
> around
> > in case the FFVD doesn't need a decryptor (or doesn't support decryption).
> > 
> > For example, this CL will break player_x11, so we need to pass another NULL
in
> > the ctor of FFVD there too.
> > 
> > scherkus, what's your latest opinion on this?
> 
> I'm indifferent -- I imagine we'll be shuffling around who owns the decryptor
> more in the future. If this gets us closer to cleaning up initialization +
> ownership I'm cool with acolwell's CL

In the future, the VRB will own the decryptor and the video decoder should know
nothing about the decryptor. So decryptor related changes in this CL will be
irrelevant. 

I am okay either way. acolwell@, do you still want to commit this? If so, I'll
review.

[acolwell GONE FROM CHROMIUM, 2012-08-06 20:50:25]

On 2012/08/06 20:46:20, xhwang wrote:
> On 2012/08/06 20:40:27, scherkus wrote:
> > On 2012/08/06 20:05:11, xhwang wrote:
> > > +scherkus for review.
> > > 
> > > Thanks for the patch. Passing the decryptor in the constructor of FFVD is
> one
> > of
> > > the original approaches I tried. It's okay for me. The reason that we
chose
> > the
> > > current implementation, IIRC, was that we didn't like to pass NULL's all
> > around
> > > in case the FFVD doesn't need a decryptor (or doesn't support decryption).
> > > 
> > > For example, this CL will break player_x11, so we need to pass another
NULL
> in
> > > the ctor of FFVD there too.
> > > 
> > > scherkus, what's your latest opinion on this?
> > 
> > I'm indifferent -- I imagine we'll be shuffling around who owns the
decryptor
> > more in the future. If this gets us closer to cleaning up initialization +
> > ownership I'm cool with acolwell's CL
> 
> In the future, the VRB will own the decryptor and the video decoder should
know
> nothing about the decryptor. So decryptor related changes in this CL will be
> irrelevant. 
> 
> I am okay either way. acolwell@, do you still want to commit this? If so, I'll
> review.

Yes. I'd like to commit this because it will make it easier for me to move
decoder init into VRB.

[xhwang, 2012-08-06 20:58:27]

lgtm w/ nits.

http://codereview.chromium.org/10834183/diff/4001/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test_base.h (right):

http://codereview.chromium.org/10834183/diff/4001/media/filters/pipeline_inte...
media/filters/pipeline_integration_test_base.h:56: ChunkDemuxerClient* client,
Decryptor* decryptor);
nit: Forward-declare Decryptor in this file?

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_wtl/movi...
File media/tools/player_wtl/movie.cc (right):

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_wtl/movi...
media/tools/player_wtl/movie.cc:83: "VideoDecoderThread"), NULL));
nit: Put NULL at the indentation as base::Bind?

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_x11/play...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_x11/play...
media/tools/player_x11/player_x11.cc:125: "VideoDecoderThread"), NULL));
nit: ditto about indentation.

[acolwell GONE FROM CHROMIUM, 2012-08-06 21:53:56]

http://codereview.chromium.org/10834183/diff/4001/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test_base.h (right):

http://codereview.chromium.org/10834183/diff/4001/media/filters/pipeline_inte...
media/filters/pipeline_integration_test_base.h:56: ChunkDemuxerClient* client,
Decryptor* decryptor);
On 2012/08/06 20:58:27, xhwang wrote:
> nit: Forward-declare Decryptor in this file?

Done.

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_wtl/movi...
File media/tools/player_wtl/movie.cc (right):

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_wtl/movi...
media/tools/player_wtl/movie.cc:83: "VideoDecoderThread"), NULL));
On 2012/08/06 20:58:27, xhwang wrote:
> nit: Put NULL at the indentation as base::Bind?

Done.

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_x11/play...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/10834183/diff/4001/media/tools/player_x11/play...
media/tools/player_x11/player_x11.cc:125: "VideoDecoderThread"), NULL));
On 2012/08/06 20:58:27, xhwang wrote:
> nit: ditto about indentation.

Done.