Media Galleries: On Linux, write data to snapshots in chunks rather than at once.

chrome/browser/media_gallery/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_gallery/mtp_device_delegate_impl_linux.h"
 
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/string_util.h"
 #include "base/synchronization/cancellation_flag.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/media_transfer_protocol/media_transfer_protocol_manager.h"
 #include "chrome/browser/media_transfer_protocol/mtp_file_entry.pb.h"
@@ skipping 328 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(GetFileInfoWorker);
 };
 
 // Worker class to read media device file data given a file |path|.
 class ReadFileWorker
     : public RefCountedThreadSafe<ReadFileWorker, ReadFileWorkerDeleter> {
  public:
   // Constructed on |media_task_runner_| thread.
   ReadFileWorker(const std::string& handle,
-                 const std::string& path,
+                 const std::string& src_path,
                  uint32 total_size,
+                 const FilePath& dest_path,
                  SequencedTaskRunner* task_runner,
                  WaitableEvent* task_completed_event,
                  WaitableEvent* shutdown_event)
       : device_handle_(handle),
-        path_(path),
+        src_path_(src_path),
         total_bytes_(total_size),
+        dest_path_(dest_path),
+        dest_fd_(-1),
+        bytes_read_(0),
         error_occurred_(false),
         media_task_runner_(task_runner),
         on_task_completed_event_(task_completed_event),
         on_shutdown_event_(shutdown_event) {
     DCHECK(on_task_completed_event_);
     DCHECK(on_shutdown_event_);
   }
 
   // This function is invoked on |media_task_runner_| to post the task on UI
   // thread. This blocks the |media_task_runner_| until the task is complete.
   void Run() {
     if (on_shutdown_event_->IsSignaled()) {
       // Process is in shutdown mode.
       // Do not post any task on |media_task_runner_|.
       return;
     }
 
-    while (!error_occurred_ && (data_.size() < total_bytes_) &&
-           !cancel_tasks_flag_.IsSet()) {
+    dest_fd_ = open(dest_path_.value().c_str(), O_WRONLY);
+    if (dest_fd_ < 0)
+      return;
+    dest_fd_scoper_.reset(&dest_fd_);
+
+    while (!error_occurred_ && (bytes_read_ < total_bytes_)) {
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                               Bind(&ReadFileWorker::DoWorkOnUIThread, this));
       on_task_completed_event_->Wait();
-      if (on_shutdown_event_->IsSignaled())
+      if (on_shutdown_event_->IsSignaled()) {
         cancel_tasks_flag_.Set();
+        break;
+      }

[kmadhusu, 2012/11/30 02:05:12]

When the MTR resumes, if |error_occured_| is true, we should break. 

[Lei Zhang, 2012/11/30 02:30:55]

Done.

+      int bytes_written =
+          file_util::WriteFileDescriptor(dest_fd_, data_.data(), data_.size());
+      if (static_cast<int>(data_.size()) != bytes_written) {
+        error_occurred_ = true;
+        break;
+      }
+      bytes_read_ += data_.size();
     }
   }
 
-  // Returns the media file contents received from mtpd.
-  const std::string& data() const { return data_; }
+  bool Succeeded() const {
+    return !error_occurred_ && bytes_read_ == total_bytes_;

[kmadhusu, 2012/11/30 02:05:12]

nit: () around binary expressions.

[Lei Zhang, 2012/11/30 02:30:55]

Done.

+  }
 
   // Returns the |media_task_runner_| associated with this worker object.
   // This function is exposed for WorkerDeleter struct to access the
   // |media_task_runner_|.
   SequencedTaskRunner* media_task_runner() const {
     return media_task_runner_.get();
   }
 
  private:
   friend struct WorkerDeleter<ReadFileWorker>;
   friend class DeleteHelper<ReadFileWorker>;
   friend class RefCountedThreadSafe<ReadFileWorker, ReadFileWorkerDeleter>;
 
   // Destructed via ReadFileWorkerDeleter.
   virtual ~ReadFileWorker() {
     // This object must be destructed on |media_task_runner_|.
   }
 
   // Dispatches a request to MediaTransferProtocolManager to get the media file
   // contents.
   void DoWorkOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (cancel_tasks_flag_.IsSet())
       return;
 
     GetMediaTransferProtocolManager()->ReadFileChunkByPath(
-        device_handle_, path_, data_.size(), BytesToRead(),
+        device_handle_, src_path_, bytes_read_, BytesToRead(),
         Bind(&ReadFileWorker::OnDidWorkOnUIThread, this));
   }
 
   // Query callback for DoWorkOnUIThread(). On success, |data| has the media
   // file contents. On failure, |error| is set to true. This function signals
   // to unblock |media_task_runner_|.
   void OnDidWorkOnUIThread(const std::string& data, bool error) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (cancel_tasks_flag_.IsSet())
       return;
 
-    error_occurred_ = error;
-    if (!error) {
-      if ((BytesToRead() == data.size())) {
-        // TODO(kmadhusu): Data could be really huge. Consider passing data by
-        // pointer/ref rather than by value here to avoid an extra data copy.
-        data_.append(data);
-      } else {
-        NOTREACHED();
-        error_occurred_ = true;
-      }
-    }
+    error_occurred_ = error || (data.size() != BytesToRead());
+    if (!error_occurred_)

[kmadhusu, 2012/11/30 02:05:12]

If |error_occurred_| is true, we should reset |data_|. If not, we will write the
previous value of |data_| in line 401.

[Lei Zhang, 2012/11/30 02:30:55]

I fixed the checks above so that won't happen.

+      data_ = data;
     on_task_completed_event_->Signal();
   }
 
   uint32 BytesToRead() const {
     // Read data in 1 MB chunks.
     static const uint32 kReadChunkSize = 1024 * 1024;
-    return std::min(kReadChunkSize,
-                    total_bytes_ - static_cast<uint32>(data_.size()));
+    return std::min(kReadChunkSize, total_bytes_ - bytes_read_);
   }
 
   // The device unique identifier to query the device.
   const std::string device_handle_;
 
   // The media device file path.
-  const std::string path_;
-
-  // The data from mtpd.
-  std::string data_;
+  const std::string src_path_;
 
   // Number of bytes to read.
   const uint32 total_bytes_;
 
+  // Where to write the data read from the device.
+  const FilePath dest_path_;
+
+  // File descriptor for |dest_path_|.
+  int dest_fd_;
+
+  // Scoper to cleanup |dest_fd_|.
+  file_util::ScopedFD dest_fd_scoper_;

[kmadhusu, 2012/11/30 02:05:12]

Is there any specific reason to have |dest_fd_| and |dest_fd_scoper_| as member
variables instead of local variables in Run()?

[Lei Zhang, 2012/11/30 02:30:55]

Not needed any more. Removed.

+
+  /*****************************************************************************
+   * The variables below are accessed on both |media_task_runner_| and the UI
+   * thread. However, there's no concurrent access because the UI thread is in a
+   * blocked state when access occurs on |media_task_runner_|.
+   */
+
+  // Number of bytes read from the device.
+  uint32 bytes_read_;
+
+  // Temporary data storage.
+  std::string data_;
+
   // Whether an error occurred during file transfer.
   bool error_occurred_;
 
+  /****************************************************************************/
+
   // A reference to |media_task_runner_| to destruct this object on the correct
   // thread.
   scoped_refptr<SequencedTaskRunner> media_task_runner_;
 
   // |media_task_runner_| can wait on this event until the required operation
   // is complete.
   // TODO(kmadhusu): Remove this WaitableEvent after modifying the
   // DeviceMediaFileUtil functions as asynchronous functions.
   WaitableEvent* on_task_completed_event_;
 
@@ skipping 377 matching lines @@
   PlatformFileError error = GetFileInfo(device_file_path, file_info);
   if (error != base::PLATFORM_FILE_OK)
     return error;
 
   if (file_info->size <= 0 || file_info->size > kuint32max)
     return base::PLATFORM_FILE_ERROR_FAILED;
 
   scoped_refptr<ReadFileWorker> worker(new ReadFileWorker(
       device_handle_,
       GetDeviceRelativePath(device_path_, device_file_path.value()),
-      file_info->size,
+      file_info->size, local_path,
       media_task_runner_, &on_task_completed_event_, &on_shutdown_event_));

[kmadhusu, 2012/11/30 02:05:12]

nit: some of these params can fit in the previous line.

[Lei Zhang, 2012/11/30 02:30:55]

Done.
That could have been said of the previous code too.

   worker->Run();
 
-  const std::string& file_data = worker->data();
-  int data_size = static_cast<int>(file_data.length());
-  if (file_data.empty() ||
-      file_util::WriteFile(local_path, file_data.c_str(),
-                           data_size) != data_size) {
+  if (!worker->Succeeded())
     return base::PLATFORM_FILE_ERROR_FAILED;
-  }
 
   // Modify the last modified time to null. This prevents the time stamp
   // verfication in LocalFileStreamReader.
   file_info->last_modified = base::Time();
-  return error;
+  return base::PLATFORM_FILE_OK;
 }
 
 SequencedTaskRunner* MTPDeviceDelegateImplLinux::GetMediaTaskRunner() {
   return media_task_runner_.get();
 }
 
 void MTPDeviceDelegateImplLinux::CancelPendingTasksAndDeleteDelegate() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // Caution: This function is called on the IO thread. Access only the thread
   // safe member variables in this function. Do all the clean up operations in
@@ skipping 40 matching lines @@
 }
 
 void MTPDeviceDelegateImplLinux::DeleteDelegateOnTaskRunner() {
   DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           Bind(&CloseStorageOnUIThread, device_handle_));
   delete this;
 }
 
 }  // namespace chrome