MTP: Use a single packet for the data phase

instead of sending 12 byte header in a separate packet.
PTP on the Mac is much happier with this approach.

Change-Id: I7d1ca498f6346afd88876d24332187b466fc469c
Signed-off-by: Mike Lockwood <lockwood@android.com>

diff --git a/media/mtp/MtpDataPacket.cpp b/media/mtp/MtpDataPacket.cpp
index 817eac0..20225ba 100644 (file)
--- a/media/mtp/MtpDataPacket.cpp
+++ b/media/mtp/MtpDataPacket.cpp
@@ -345,56 +345,28 @@ void MtpDataPacket::putString(const uint16_t* string) {
 
 #ifdef MTP_DEVICE 
 int MtpDataPacket::read(int fd) {
-    // first read the header
-    int ret = ::read(fd, mBuffer, MTP_CONTAINER_HEADER_SIZE);
-    if (ret != MTP_CONTAINER_HEADER_SIZE)
-        return -1;
-    // then the following data
-    int total = MtpPacket::getUInt32(MTP_CONTAINER_LENGTH_OFFSET);
-    allocate(total);
-    int remaining = total - MTP_CONTAINER_HEADER_SIZE;
-    ret = ::read(fd, &mBuffer[0] + MTP_CONTAINER_HEADER_SIZE, remaining);
-    if (ret != remaining)
+    int ret = ::read(fd, mBuffer, mBufferSize);
+    if (ret < MTP_CONTAINER_HEADER_SIZE)
         return -1;
-
-    mPacketSize = total;
+    mPacketSize = ret;
     mOffset = MTP_CONTAINER_HEADER_SIZE;
-    return total;
-}
-
-int MtpDataPacket::readDataHeader(int fd) {
-    int ret = ::read(fd, mBuffer, MTP_CONTAINER_HEADER_SIZE);
-    if (ret > 0)
-        mPacketSize = ret;
-    else
-        mPacketSize = 0;
     return ret;
 }
 
 int MtpDataPacket::write(int fd) {
     MtpPacket::putUInt32(MTP_CONTAINER_LENGTH_OFFSET, mPacketSize);
     MtpPacket::putUInt16(MTP_CONTAINER_TYPE_OFFSET, MTP_CONTAINER_TYPE_DATA);
-    // send header separately from data
-    int ret = ::write(fd, mBuffer, MTP_CONTAINER_HEADER_SIZE);
-    if (ret == MTP_CONTAINER_HEADER_SIZE)
-        ret = ::write(fd, mBuffer + MTP_CONTAINER_HEADER_SIZE,
-                        mPacketSize - MTP_CONTAINER_HEADER_SIZE);
-    return (ret < 0 ? ret : 0);
-}
-
-int MtpDataPacket::writeDataHeader(int fd, uint32_t length) {
-    MtpPacket::putUInt32(MTP_CONTAINER_LENGTH_OFFSET, length);
-    MtpPacket::putUInt16(MTP_CONTAINER_TYPE_OFFSET, MTP_CONTAINER_TYPE_DATA);
-    int ret = ::write(fd, mBuffer, MTP_CONTAINER_HEADER_SIZE);
+    int ret = ::write(fd, mBuffer, mPacketSize);
     return (ret < 0 ? ret : 0);
 }
 
 int MtpDataPacket::writeData(int fd, void* data, uint32_t length) {
-    MtpPacket::putUInt32(MTP_CONTAINER_LENGTH_OFFSET, length + MTP_CONTAINER_HEADER_SIZE);
+    allocate(length);
+    memcpy(mBuffer + MTP_CONTAINER_HEADER_SIZE, data, length);
+    length += MTP_CONTAINER_HEADER_SIZE;
+    MtpPacket::putUInt32(MTP_CONTAINER_LENGTH_OFFSET, length);
     MtpPacket::putUInt16(MTP_CONTAINER_TYPE_OFFSET, MTP_CONTAINER_TYPE_DATA);
-    int ret = ::write(fd, mBuffer, MTP_CONTAINER_HEADER_SIZE);
-    if (ret == MTP_CONTAINER_HEADER_SIZE)
-        ret = ::write(fd, data, length);
+    int ret = ::write(fd, mBuffer, length);
     return (ret < 0 ? ret : 0);
 }
 

diff --git a/media/mtp/MtpDataPacket.h b/media/mtp/MtpDataPacket.h
index 8a08948..2b81063 100644 (file)
--- a/media/mtp/MtpDataPacket.h
+++ b/media/mtp/MtpDataPacket.h
@@ -41,6 +41,7 @@ public:
     void                setOperationCode(MtpOperationCode code);
     void                setTransactionID(MtpTransactionID id);
 
+    inline const uint8_t*     getData() const { return mBuffer + MTP_CONTAINER_HEADER_SIZE; }
     inline uint8_t      getUInt8() { return (uint8_t)mBuffer[mOffset++]; }
     inline int8_t       getInt8() { return (int8_t)mBuffer[mOffset++]; }
     uint16_t            getUInt16();
@@ -95,11 +96,9 @@ public:
 #ifdef MTP_DEVICE
     // fill our buffer with data from the given file descriptor
     int                 read(int fd);
-    int                 readDataHeader(int fd);
 
     // write our data to the given file descriptor
     int                 write(int fd);
-    int                 writeDataHeader(int fd, uint32_t length);
     int                 writeData(int fd, void* data, uint32_t length);
 #endif
 

diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp
index 4047e2e..a9b539b 100644 (file)
--- a/media/mtp/MtpServer.cpp
+++ b/media/mtp/MtpServer.cpp
@@ -731,14 +731,12 @@ MtpResponseCode MtpServer::doGetObject() {
     }
     mfr.offset = 0;
     mfr.length = fileLength;
-
-    // send data header
-    mData.setOperationCode(mRequest.getOperationCode());
-    mData.setTransactionID(mRequest.getTransactionID());
-    mData.writeDataHeader(mFD, fileLength + MTP_CONTAINER_HEADER_SIZE);
+    mfr.command = mRequest.getOperationCode();
+    mfr.transaction_id = mRequest.getTransactionID();
 
     // then transfer the file
-    int ret = ioctl(mFD, MTP_SEND_FILE, (unsigned long)&mfr);
+    int ret = ioctl(mFD, MTP_SEND_FILE_WITH_HEADER, (unsigned long)&mfr);
+    LOGV("MTP_SEND_FILE_WITH_HEADER returned %d\n", ret);
     close(mfr.fd);
     if (ret < 0) {
         if (errno == ECANCELED)
@@ -798,15 +796,13 @@ MtpResponseCode MtpServer::doGetPartialObject(MtpOperationCode operation) {
     }
     mfr.offset = offset;
     mfr.length = length;
+    mfr.command = mRequest.getOperationCode();
+    mfr.transaction_id = mRequest.getTransactionID();
     mResponse.setParameter(1, length);
 
-    // send data header
-    mData.setOperationCode(mRequest.getOperationCode());
-    mData.setTransactionID(mRequest.getTransactionID());
-    mData.writeDataHeader(mFD, length + MTP_CONTAINER_HEADER_SIZE);
-
-    // then transfer the file
-    int ret = ioctl(mFD, MTP_SEND_FILE, (unsigned long)&mfr);
+    // transfer the file
+    int ret = ioctl(mFD, MTP_SEND_FILE_WITH_HEADER, (unsigned long)&mfr);
+    LOGV("MTP_SEND_FILE_WITH_HEADER returned %d\n", ret);
     close(mfr.fd);
     if (ret < 0) {
         if (errno == ECANCELED)
@@ -918,7 +914,7 @@ MtpResponseCode MtpServer::doSendObject() {
         return MTP_RESPONSE_GENERAL_ERROR;
     MtpResponseCode result = MTP_RESPONSE_OK;
     mode_t mask;
-    int ret;
+    int ret, initialData;
 
     if (mSendObjectHandle == kInvalidObjectHandle) {
         LOGE("Expected SendObjectInfo before SendObject");
@@ -926,12 +922,13 @@ MtpResponseCode MtpServer::doSendObject() {
         goto done;
     }
 
-    // read the header
-    ret = mData.readDataHeader(mFD);
-    // FIXME - check for errors here.
-
-    // reset so we don't attempt to send this back
-    mData.reset();
+    // read the header, and possibly some data
+    ret = mData.read(mFD);
+    if (ret < MTP_CONTAINER_HEADER_SIZE) {
+        result = MTP_RESPONSE_GENERAL_ERROR;
+        goto done;
+    }
+    initialData = ret - MTP_CONTAINER_HEADER_SIZE;
 
     mtp_file_range  mfr;
     mfr.fd = open(mSendObjectFilePath, O_RDWR | O_CREAT | O_TRUNC);
@@ -945,15 +942,19 @@ MtpResponseCode MtpServer::doSendObject() {
     fchmod(mfr.fd, mFilePermission);
     umask(mask);
 
-    mfr.offset = 0;
-    mfr.length = mSendObjectFileSize;
+    if (initialData > 0)
+        ret = write(mfr.fd, mData.getData(), initialData);
 
-    LOGV("receiving %s\n", (const char *)mSendObjectFilePath);
-    // transfer the file
-    ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr);
-    close(mfr.fd);
+    if (mSendObjectFileSize - initialData > 0) {
+        mfr.offset = initialData;
+        mfr.length = mSendObjectFileSize - initialData;
 
-    LOGV("MTP_RECEIVE_FILE returned %d", ret);
+        LOGV("receiving %s\n", (const char *)mSendObjectFilePath);
+        // transfer the file
+        ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr);
+        LOGV("MTP_RECEIVE_FILE returned %d\n", ret);
+    }
+    close(mfr.fd);
 
     if (ret < 0) {
         unlink(mSendObjectFilePath);
@@ -964,6 +965,9 @@ MtpResponseCode MtpServer::doSendObject() {
     }
 
 done:
+    // reset so we don't attempt to send the data back
+    mData.reset();
+
     mDatabase->endSendObject(mSendObjectFilePath, mSendObjectHandle, mSendObjectFormat,
             result == MTP_RESPONSE_OK);
     mSendObjectHandle = kInvalidObjectHandle;
@@ -1096,23 +1100,31 @@ MtpResponseCode MtpServer::doSendPartialObject() {
         return MTP_RESPONSE_GENERAL_ERROR;
     }
 
-    // read the header
-    int ret = mData.readDataHeader(mFD);
-    // FIXME - check for errors here.
+    const char* filePath = (const char *)edit->mPath;
+    LOGV("receiving partial %s %lld %lld\n", filePath, offset, length);
 
-    // reset so we don't attempt to send this back
-    mData.reset();
+    // read the header, and possibly some data
+    int ret = mData.read(mFD);
+    if (ret < MTP_CONTAINER_HEADER_SIZE)
+        return MTP_RESPONSE_GENERAL_ERROR;
+    int initialData = ret - MTP_CONTAINER_HEADER_SIZE;
 
-    const char* filePath = (const char *)edit->mPath;
-    LOGV("receiving partial %s %lld %ld\n", filePath, offset, length);
-    mtp_file_range  mfr;
-    mfr.fd = edit->mFD;
-    mfr.offset = offset;
-    mfr.length = length;
+    if (initialData > 0) {
+        ret = write(edit->mFD, mData.getData(), initialData);
+        offset += initialData;
+        length -= initialData;
+    }
 
-    // transfer the file
-    ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr);
-    LOGV("MTP_RECEIVE_FILE returned %d", ret);
+    if (length > 0) {
+        mtp_file_range  mfr;
+        mfr.fd = edit->mFD;
+        mfr.offset = offset;
+        mfr.length = length;
+
+        // transfer the file
+        ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr);
+        LOGV("MTP_RECEIVE_FILE returned %d", ret);
+    }
     if (ret < 0) {
         mResponse.setParameter(1, 0);
         if (errno == ECANCELED)
@@ -1120,6 +1132,9 @@ MtpResponseCode MtpServer::doSendPartialObject() {
         else
             return MTP_RESPONSE_GENERAL_ERROR;
     }
+
+    // reset so we don't attempt to send this back
+    mData.reset();
     mResponse.setParameter(1, length);
     uint64_t end = offset + length;
     if (end > edit->mSize) {