MTP Streaming: Optimize block reading

MTP Streaming: Optimize block reading

Previously, due to some unoptimal code, we were doing two mtp stats per block.
This cuts it down to one mtp stat per block. Still not quite there, but working on it...

BUG=110119
TBR=thestig@chromium.org

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

[vandebo (ex-Chrome), 2014-02-27 17:20:50]

LGTM

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/fileapi/mtp_file_stream_reader.cc (right):

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/fileapi/mtp_file_stream_reader.cc:44: void
ReadBytes(
You might be able to eliminate this function now.

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:473:
request.offset >= file_info.size) {
Should this be > instead of >=.  It's possible that the file is a size that we
read right up to the last byte in the previous request - or am I having an off
by one error?

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:480: int
bytes_to_read = std::min(
You might be able to push the file info validation down until the task_helper
layer and that way you wouldn't need to break up the request struct, but if not,
what ever.

[tommycli, 2014-02-27 18:57:33]

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/fileapi/mtp_file_stream_reader.cc (right):

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/fileapi/mtp_file_stream_reader.cc:44: void
ReadBytes(
On 2014/02/27 17:20:50, vandebo wrote:
> You might be able to eliminate this function now.

Will probably do this in the next CL (which refactors this class).

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:473:
request.offset >= file_info.size) {
On 2014/02/27 17:20:50, vandebo wrote:
> Should this be > instead of >=.  It's possible that the file is a size that we
> read right up to the last byte in the previous request - or am I having an off
> by one error?

I think the byte indices run from 0 to size-1, so if the offset == size, we're
trying to read one past the last byte of the file.

https://codereview.chromium.org/180783005/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:480: int
bytes_to_read = std::min(
On 2014/02/27 17:20:50, vandebo wrote:
> You might be able to push the file info validation down until the task_helper
> layer and that way you wouldn't need to break up the request struct, but if
not,
> what ever.

Done.

[tommycli, 2014-02-27 19:21:56]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:24:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/180783005/40001

[vandebo (ex-Chrome), 2014-02-27 19:32:26]

https://codereview.chromium.org/180783005/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_task_helper.cc (right):

https://codereview.chromium.org/180783005/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_task_helper.cc:223:
base::checked_cast<int>(file_info.size - request.offset));
This changed from saturated_cast to checked_cast...

[tommycli, 2014-02-27 20:18:06]

https://codereview.chromium.org/180783005/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_task_helper.cc (right):

https://codereview.chromium.org/180783005/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_task_helper.cc:223:
base::checked_cast<int>(file_info.size - request.offset));
On 2014/02/27 19:32:26, vandebo wrote:
> This changed from saturated_cast to checked_cast...

I changed some of these around. I also added some DCHECKs to MTP_daemon_client.
The comments in the .h file say the chunks are limited to 1MB, but it wasn't
previously enforced.

[tommycli, 2014-02-27 20:18:19]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-02-27 20:18:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/180783005/10008

[tommycli, 2014-02-27 20:18:38]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2014-02-27 20:18:38]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2014-02-27 20:19:15]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2014-02-27 20:34:29]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-02-27 20:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/180783005/110001

[commit-bot: I haz the power, 2014-02-27 21:00:44]

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

[commit-bot: I haz the power, 2014-02-27 21:00:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tommycli, 2014-02-27 21:28:19]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-02-27 21:30:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/180783005/110001

[commit-bot: I haz the power, 2014-02-28 00:03:14]

Change committed as 253980