MTP Streaming: Optimize block reading

chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/media_galleries/linux/snapshot_file_details.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/io_buffer.h"
 
 namespace {
 
 // File path separator constant.
@@ skipping 125 matching lines @@
 
 // Copies the contents of |device_file_path| to |snapshot_file_path|.
 //
 // Called on the UI thread to dispatch the request to the
 // MediaTransferProtocolManager.
 //
 // |storage_name| specifies the name of the storage device.
 // |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
 // MTPDeviceTaskHelper object associated with the device storage.
 //
 // Called on the UI thread to dispatch the request to the
 // MediaTransferProtocolManager.
 void CloseStorageAndDestroyTaskHelperOnUIThread(
     const std::string& storage_name) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ skipping 284 matching lines @@
 void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToReadBytes(
     const ReadBytesRequest& request,
     const base::File::Info& file_info) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   DCHECK(request.buf);
   DCHECK(request.buf_len >= 0);
   DCHECK(task_in_progress_);
   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,
                  weak_ptr_factory_.GetWeakPtr(), request.error_callback));
 
   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(
     const ReadDirectorySuccessCallback& success_callback,
     const fileapi::AsyncFileUtil::EntryList& file_list) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   success_callback.Run(file_list, false /*no more entries*/);
   task_in_progress_ = false;
   ProcessNextPendingRequest();
 }
@@ skipping 17 matching lines @@
   DCHECK(current_snapshot_request_info_.get());
   DCHECK(task_in_progress_);
   current_snapshot_request_info_->error_callback.Run(error);
   task_in_progress_ = false;
   current_snapshot_request_info_.reset();
   ProcessNextPendingRequest();
 }
 
 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();
 }
 
 void MTPDeviceDelegateImplLinux::HandleDeviceFileError(
     const ErrorCallback& error_callback,
     base::File::Error error) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   DCHECK(task_in_progress_);
   error_callback.Run(error);
   task_in_progress_ = false;
   ProcessNextPendingRequest();
 }
 
 void CreateMTPDeviceAsyncDelegate(
     const std::string& device_location,
     const CreateMTPDeviceAsyncDelegateCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   callback.Run(new MTPDeviceDelegateImplLinux(device_location));
 }