MTP Streaming: Optimize block reading

chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc

Index: chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
diff --git a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
index 50f6291400d111d00acb186aba83813b121fb75e..5cab51cb7739fb28b56546d88a68bc8c1cc510ee 100644
--- a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
+++ b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
@@ -4,8 +4,11 @@
 
 #include "chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/files/file_path.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/media_galleries/linux/mtp_device_task_helper.h"
 #include "chrome/browser/media_galleries/linux/mtp_device_task_helper_map_service.h"
@@ -151,13 +154,17 @@ void WriteDataIntoSnapshotFileOnUIThread(
 // |request| is a struct containing details about the byte read request.
 void ReadBytesOnUIThread(
     const std::string& storage_name,
-    const MTPDeviceAsyncDelegate::ReadBytesRequest& request) {
+    const MTPDeviceAsyncDelegate::ReadBytesRequest& request,
+    const base::File::Info& file_info) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   MTPDeviceTaskHelper* task_helper =
       GetDeviceTaskHelperForStorage(storage_name);
   if (!task_helper)
     return;
-  task_helper->ReadBytes(request);
+  task_helper->ReadBytes(request.device_file_relative_path, request.buf,
+                         request.offset, request.buf_len,
+                         base::Bind(request.success_callback, file_info),
+                         request.error_callback);
 }
 
 // Closes the device storage specified by the |storage_name| and destroys the
@@ -462,17 +469,23 @@ void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToReadBytes(
   base::File::Error error = base::File::FILE_OK;
   if (file_info.is_directory)
     error = base::File::FILE_ERROR_NOT_A_FILE;
-  else if (file_info.size < 0 || file_info.size > kuint32max)
+  else if (file_info.size < 0 || file_info.size > kuint32max ||
+           request.offset >= file_info.size) {

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

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?

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

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.

     error = base::File::FILE_ERROR_FAILED;
+  }
 
   if (error != base::File::FILE_OK)
     return HandleDeviceFileError(request.error_callback, error);
 
+  int bytes_to_read = std::min(

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

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]

Done.

+      request.buf_len,
+      base::checked_cast<int>(file_info.size - request.offset));
+
   ReadBytesRequest new_request(
       request.device_file_relative_path,
       request.buf,
       request.offset,
-      request.buf_len,
+      bytes_to_read,
       base::Bind(&MTPDeviceDelegateImplLinux::OnDidReadBytes,
                  weak_ptr_factory_.GetWeakPtr(), request.success_callback),
       base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
@@ -481,7 +494,7 @@ void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToReadBytes(
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&ReadBytesOnUIThread, storage_name_, new_request));
+      base::Bind(&ReadBytesOnUIThread, storage_name_, new_request, file_info));
 }
 
 void MTPDeviceDelegateImplLinux::OnDidReadDirectory(
@@ -519,10 +532,10 @@ void MTPDeviceDelegateImplLinux::OnWriteDataIntoSnapshotFileError(
 
 void MTPDeviceDelegateImplLinux::OnDidReadBytes(
     const ReadBytesSuccessCallback& success_callback,
-    int bytes_read) {
+    const base::File::Info& file_info, int bytes_read) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   DCHECK(task_in_progress_);
-  success_callback.Run(bytes_read);
+  success_callback.Run(file_info, bytes_read);
   task_in_progress_ = false;
   ProcessNextPendingRequest();
 }