Refactor FFmpegURLProtocol code from FFmpegDemuxer into BlockingUrlProtocol.

Refactor FFmpegURLProtocol code from FFmpegDemuxer into BlockingUrlProtocol.

I cleaned up some comments and surrounding test code in FFmpegDemuxer and FileDataSource as well. There'll be some more clean up after I get this landed.

BUG=160640
TEST=FFmpegDemuxer and BlockingUrlProtocol tests

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

[scherkus (not reviewing), 2012-11-13 00:22:02]

dalecurtis: you are the lucky person who has happened to touch this url protocol
business stuff last!

[DaleCurtis, 2012-11-13 03:42:28]

lgtm % nits and nice to haves.

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
File media/filters/blocking_url_protocol.cc (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol.cc:35: // Even though FFmpeg defines
AVERROR_EOF, it's not to be used with I/O
+rbultje; I wonder if this is still true, Ronald?

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
File media/filters/blocking_url_protocol.h (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol.h:10: #include "base/compiler_specific.h"
Needed?

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol.h:21: // TODO(scherkus): Should be more
thread-safe and use ConditionVariable+Lock as
WaitableEvent uses CV under the hood, so I think this TODO is redundant.

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
File media/filters/blocking_url_protocol_unittest.cc (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol_unittest.cc:88:
data_source_->force_read_errors_for_testing();
OOC, are the hacker style _for_testing() methods caught by the presubmit/clang
plugin? I know the ForTesting() ones are.

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
File media/filters/file_data_source.cc (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
media/filters/file_data_source.cc:15: FileDataSource::FileDataSource()
Might be worth just replacing most of this a FileUtil::MemoryMappedFile().

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
File media/filters/file_data_source.h (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
media/filters/file_data_source.h:34: void disable_file_size_for_testing() {
disable_file_size_ = true; }
This isn't used anywhere in this CL. Remove?

[rbultje, 2012-11-13 03:52:17]

Hi,

On 2012/11/13 03:42:28, DaleCurtis wrote:
>
http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
> media/filters/blocking_url_protocol.cc:35: // Even though FFmpeg defines
> AVERROR_EOF, it's not to be used with I/O
> +rbultje; I wonder if this is still true, Ronald?

Yes. I/O simply returns 0 on eof, similar to what read(2) would return, and the
demuxer then translates that to AVERROR_EOF.

Ronald

[scherkus (not reviewing), 2012-11-13 18:19:13]

thanks!

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
File media/filters/blocking_url_protocol.h (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol.h:10: #include "base/compiler_specific.h"
On 2012/11/13 03:42:28, DaleCurtis wrote:
> Needed?

Done.

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol.h:21: // TODO(scherkus): Should be more
thread-safe and use ConditionVariable+Lock as
On 2012/11/13 03:42:28, DaleCurtis wrote:
> WaitableEvent uses CV under the hood, so I think this TODO is redundant.

WEs are implemented using OS primitives (e.g., HANDLE on windows) and CVs are
implemented using events when the OS does not provide a primitive (e.g., WinXP
implementation of a CV)

Anyway, I updated the TODO to describe the problem as opposed to a potential
solution

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
File media/filters/blocking_url_protocol_unittest.cc (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/blocking_url_...
media/filters/blocking_url_protocol_unittest.cc:88:
data_source_->force_read_errors_for_testing();
On 2012/11/13 03:42:28, DaleCurtis wrote:
> OOC, are the hacker style _for_testing() methods caught by the presubmit/clang
> plugin? I know the ForTesting() ones are.

Sure does! I tried it out on some sample code.

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  media/filters/ffmpeg_demuxer.cc:478
    fds->disable_file_size_for_testing();

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
File media/filters/file_data_source.cc (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
media/filters/file_data_source.cc:15: FileDataSource::FileDataSource()
On 2012/11/13 03:42:28, DaleCurtis wrote:
> Might be worth just replacing most of this a FileUtil::MemoryMappedFile().

I'll do a follow up CL

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
File media/filters/file_data_source.h (right):

http://codereview.chromium.org/11410052/diff/2001/media/filters/file_data_sou...
media/filters/file_data_source.h:34: void disable_file_size_for_testing() {
disable_file_size_ = true; }
On 2012/11/13 03:42:28, DaleCurtis wrote:
> This isn't used anywhere in this CL. Remove?

Done.

[commit-bot: I haz the power, 2012-11-13 18:19:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/11410052/21

[commit-bot: I haz the power, 2012-11-13 18:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/11410052/6003

[commit-bot: I haz the power, 2012-11-13 18:45:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[DaleCurtis, 2012-11-13 21:44:07]

https://chromiumcodereview.appspot.com/11410052/diff/2001/media/filters/block...
File media/filters/blocking_url_protocol.h (right):

https://chromiumcodereview.appspot.com/11410052/diff/2001/media/filters/block...
media/filters/blocking_url_protocol.h:21: // TODO(scherkus): Should be more
thread-safe and use ConditionVariable+Lock as
On 2012/11/13 18:19:13, scherkus wrote:
> On 2012/11/13 03:42:28, DaleCurtis wrote:
> > WaitableEvent uses CV under the hood, so I think this TODO is redundant.
> 
> WEs are implemented using OS primitives (e.g., HANDLE on windows) and CVs are
> implemented using events when the OS does not provide a primitive (e.g., WinXP
> implementation of a CV)
> 
> Anyway, I updated the TODO to describe the problem as opposed to a potential
> solution

Whoops, I was looking at the internal posix SyncWaiter implementation. You're
correct.

https://chromiumcodereview.appspot.com/11410052/diff/6003/media/filters/block...
File media/filters/blocking_url_protocol.cc (right):

https://chromiumcodereview.appspot.com/11410052/diff/6003/media/filters/block...
media/filters/blocking_url_protocol.cc:43: data_source_->Read(read_position_,
size, data, base::Bind(
I goofed here and misunderstood your original comment on this and didn't realize
last_read_bytes_ was actually getting written by multiple threads.

Why not instead use a global abort event plus a stack allocated waitable event
and last_read_bytes value bound into the callback?  You can then use WaitMany()
to listen for either one to fire and take the appropriate action.

[scherkus (not reviewing), 2012-11-13 21:56:13]

https://chromiumcodereview.appspot.com/11410052/diff/6003/media/filters/block...
File media/filters/blocking_url_protocol.cc (right):

https://chromiumcodereview.appspot.com/11410052/diff/6003/media/filters/block...
media/filters/blocking_url_protocol.cc:43: data_source_->Read(read_position_,
size, data, base::Bind(
On 2012/11/13 21:44:07, DaleCurtis wrote:
> I goofed here and misunderstood your original comment on this and didn't
realize
> last_read_bytes_ was actually getting written by multiple threads.
> 
> Why not instead use a global abort event plus a stack allocated waitable event
> and last_read_bytes value bound into the callback?  You can then use
WaitMany()
> to listen for either one to fire and take the appropriate action.

I have a queue of CLs. Sounds like you're ready to review the next one :)

http://codereview.chromium.org/11360237/