Closed Bug 1039939 Opened 7 years ago Closed 7 years ago

[MTP] enable the functionality on windows 7

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S1 (1aug)
feature-b2g 2.1

People

(Reporter: alchen, Assigned: alchen)

References

Details

(Whiteboard: [p=8] [ft:devices])

Attachments

(1 file, 1 obsolete file)

In current status, we can see a portable device called flame.
There is a volume called "firefox os" when entering this portable device.
However, we cannot see anything when entering "firefox os" volume.

The following is the MTP command which we received with no error response.
But the command just stop after "MTP_OPERATION_GET_OBJECT_INFO".

MTP_OPERATION_OPEN_SESSION -> MTP_OPERATION_GET_DEVICE_INFO -> 
MTP_OPERATION_GET_OBJECT_PROPS_SUPPORTED ->
MTP_OPERATION_GET_STORAGE_IDS -> MTP_OPERATION_GET_STORAGE_INFO -> MTP_OPERATION_GET_OBJECT_HANDLES -> MTP_OPERATION_GET_OBJECT_PROP_DESC -> MTP_OPERATION_GET_OBJECT_PROP_LIST ->
MTP_OPERATION_GET_OBJECT_INFO
I try to do modification on the response for MTP_OPERATION_GET_OBJECT_INFO and let it similar as what I see on Nexus 4 case. However, I still cannot see anything (after enter "firefox" volume) with this change.
So I will move forward on previous response.

Next target : "MTP_OPERATION_GET_OBJECT_PROP_LIST"
Update the new finding,
we can see folders with the following changes when entering "FirefoxOS" volume.


 MtpObjectFormatList*
 MozMtpDatabase::getSupportedPlaybackFormats()
 {
-  static const uint16_t init_data[] = {MTP_FORMAT_PNG};
+  static const uint16_t init_data[] = {MTP_FORMAT_UNDEFINED, MTP_FORMAT_ASSOCIATION, MTP_FORMAT_PNG};

However, the folder looks weird.
It show "local disk" without filename.

I also try to add the implementation of "getObjectReferences" but in vain.
Whiteboard: [p=8]
Blocks: mtp
Assignee: nobody → alchen
feature-b2g: --- → 2.1
Whiteboard: [p=8] → [p=8] [ft:Devices]
Target Milestone: --- → 2.1 S2 (15aug)
Whiteboard: [p=8] [ft:Devices] → [p=8] [ft:devices]
With this patch, W7 have the same ability as Ubuntu.

Summarize this patch as below:

1. Add "MTP_FORMAT_UNDEFINED" and "MTP_FORMAT_ASSOCIATION" into the list of "SupportedPlaybackFormats". Without them, I even cannot see sdcard folder when entering "FirefoxOS" volume.

2. From MTP specification, "MTP_PROPERTY_NAME" and "MTP_PROPERTY_PERSISTENT_UID" is required. Add the support of these two properties.

3. Fix bug in "getObjectPropertyValue": different properties should have different type.

4. Fix bug in "getObjectPropertyList": the first data we need to reply should be the mount of properties we reply in the packet. (numObjectProperties * numEntries)

5. Correct the some object information in "getObjectInfo".
Attachment #8463834 - Flags: review?(dhylands)
Comment on attachment 8463834 [details] [diff] [review]
(0729) Enable MTP functionality on WIN7

Review of attachment 8463834 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

The only thing I think I'd do (as commented) is to combine the storage ID and handle together in order to create the UID.

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +365,2 @@
>      case MTP_PROPERTY_DISPLAY_NAME:   aPacket.putString(entry->mDisplayName.get()); break;
> +    case MTP_PROPERTY_PERSISTENT_UID: aPacket.putUInt128(entry->mHandle); break;

I think that this is probably insufficient. We should combine the storageID (in the upper 64 bits) and handle (in the lower 64 bits) together to cover the case where there are multiple storage areas.

Right now the handle is unique globally, but I think that as soon as we add support of multiple storage areas we'll wind up tracking files per storage area.
Attachment #8463834 - Flags: review?(dhylands) → review+
Based on the comment 4, there is only one modification compared to attachment 8463834 [details] [diff] [review]. 
The value of "MTP_PROPERTY_PERSISTENT_UID" is storageID in the upper 64 bits and handle in the lower 64 bits. 

Try server result. It looks fine.
https://tbpl.mozilla.org/?tree=Try&rev=dd4792495e45
Attachment #8463834 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd47316a63d2

Any particular reason the Try run was across all platforms? Based on the code being touched and the bug title, that seems quite excessive. Please try to be conscientious about that as "all" runs consume a LOT of resources, often unnecessarily.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bd47316a63d2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S2 (15aug) → 2.1 S1 (1aug)
You need to log in before you can comment on or make changes to this bug.