This is the beginning of MTP implementation. We'll finish MTP boilerplate here. Moreover, after this is fixed, I hope that my Ubuntu laptop will be able to recognize my Flame as a remote MTP device. p.s: not pretty sure what component would be the most suitable, so I set it to General since the user story of MTP (bug 922927) currently is set to General as well.

I spent some time on this and have had a super basic version which definitely still needs some more work and cleanup: https://github.com/eric30/gecko-dev/commit/aff1c2bf2d3910e4b32ea4393c5d49ae68e7e7ab 1) A folder 'mtp' is created under 'dom' to keep MTP related implementation. 2) MTP related code under frameworks/av/media/mtp/ are reused so that we won't need to do everything on our own. 3) The way I am using to toggle USB mass storage and MTP is turning Bluetooth on and off. Super hacky. ;) 4) If you cherry-pick the commit above, you'll find out that server->run() will end very soon. I guess it's because I didn't give MTP backend a valid 'MtpDatabase'. I'll get a formal patch for review as soon as possible. Hope we can get this done by the end of next week.

I've also been working on the same thing. I've been working on the database portion (creating an FileDatabase which is derived from MtpDatabase).

This version tries to fire up the mtp server, but it fails on the first call to mRequest.read. I think that after setting the USB interface to mtp, that we need to wait for the USB interface to be configured before we try to do I/O. I moved the code to dom/system/gonk, since I think that this is much closer to USB Mass Storage, an mtp won't have a DOM interface (so it doesn't really belong in dom/mtp). I think that the AutoMounter will do the USB cable detection, and figure out the right time to start the server (which is very similar logic to whats being done for UMS. Anyways, since this compiles and links, I thought I would attach it.

It turns out that you need to wait for the USB interface to be CONFIGURED before trying to use /dev/mtp_usb. So I hacked MTP support into the AutoMounter. To use: - build and flash flame with this patch - run the following commands: > adb root > adb shell setprop sys.usb.config mtp,adb > adb shell getprop sys.usb.config After the setprop command, the adb connection will be dopped and the device will be offline for a short period of time (3-5 seconds). Doing the getprop just confirms that setprop worked ok. Then go into settings and enable USB Mass Storage. On my Ubuntu 14.04, I see an "Android" with "FirefoxOS" show up, however double clicking doesn't show any files. You can enable lots of debug in the MtpServer code by editing frameworks/av/media/mtp/MtpDebug.h and uncommenting the following line: > // #define LOG_NDEBUG 0 It looks like the code is expecting getObjectPropertyList function to be implemented. It currently get called thusly: > getObjectPropertyList: Handle: 0x00000001 Format: 0x00000000 aProperty: 0xffffffff aGroupCode: 0 aDepth 0 (NOT SUPPORTED) I found the android implementation here: http://androidxref.com/4.4.3_r1.1/xref/frameworks/base/media/java/android/mtp/MtpDatabase.java#672 which seems to use the following Java MtpPropertyGroup code: http://androidxref.com/4.4.3_r1.1/xref/frameworks/base/media/java/android/mtp/MtpPropertyGroup.java#32 I also realized my last patch was missing a bunch of files. These should be added now.

ok - now I am able to browse around the phone (via MTP) from my linux host (running ubuntu 14.04) and I can open files. Will upload a YouTube video showing the results...

Here's a video clip showing this working: https://www.youtube.com/watch?v=6sb2qlDgQzw

(In reply to Dave Hylands [:dhylands] from comment #5) > Created attachment 8450482 [details] [diff] [review] > WIP - Able to open files via MTP > > ok - now I am able to browse around the phone (via MTP) from my linux host > (running ubuntu 14.04) and I can open files. > > Will upload a YouTube video showing the results... So efficient! I tried the patch today. MTP mostly works well on my Ubuntu 13.04 (64bit). At first I couldn't open image files like .jpg or .png but .pdf could be opened successfully. After investigation, I believe it was not our problem because the host kept sending MTP_OPERATION_GET_OBJECT_HANDLES instead of MTP_OPERATION_GET_OBJECT when I requested opening an image file. Furthermore, my Moto G had the same problem. A small problem that I found was, when Ubuntu detected the device, the title shown on my file explorer was 'Unnamed Device'. On the other hand, 'XT1032' was shown when my Moto G was attached. Dave, do you think we could get a first-version of MTP land? My idea is that, after having the infrastructure, we may have more sense about how to splitting the work into several pieces so that you guys can co-work without duplicate work.

I think it should be landable right now (pending review). The current code will only try to activate the MtpServer if sys.usb.config contains "mtp" in the string. The next thing I'll be working on is the automatic startup/failover to UMS. For that, I'm planning on introducing some properties to enable/control MTP, and as long as its off by default, then we should be able to continue working/landing stuff. Would you be able to review what's there so far?

I suspect part of the differences may be that in some cases, PTP operations are being performed, and in some cases MTP operations are being performed. Our primary target for testing should probably be Windows 7 or newer (I don't think XP supported MTP, but I'm not sure). I know I could never get MTP to work (with my nexus 4/5) on my laptop when it was running 12.04, but it seems to work fine under 14.04.

Just had a thought: the volume label may influence how it shows up on the host.

Comment on attachment 8450482 [details] [diff] [review] WIP - Able to open files via MTP Eric - putting you down to review this so far, I think it's fine to land since it won't activate mtp unless you manually run > adb shell setprop sys.usb.config mtp,adb

One of the things I'd like to change (not necessarily before we do the initial landing) is to change the scanning. Currently, it scans an entire volume and builds up an array of the files. 1 - Have the in-memory data structure be built on-demand. i.e. only scan when the user does that first double click, and only scan that topmost directory. Then only scan subdirectories as needed. 2 - We probably cache too much information right now. I think that the minimum we need to cache is the filename. Everything else can be looked up as needed. 3 - We probably want to keep the array (so we can do quick handle to object lookups), but we can also add parent/child/sibling pointers to make traversing the tree more efficient. 4 - handles only have to persist until session close, so we can drop the cache whenever the session is closed.

> Our primary target for testing should probably be Windows 7 or newer (I > don't think XP supported MTP, but I'm not sure). I know I could never get > MTP to work (with my nexus 4/5) on my laptop when it was running 12.04, but > it seems to work fine under 14.04. Yeah, I agree. Get MTP working on Windows 7 should be our primary goal (I believe that Windows XP doesn't support MTP). Just like I mentioned in the meeting this morning, Alphan will start testing on Windows 7 and we'll see what else we can do to complete this feature.

(In reply to Eric Chou [:ericchou] [:echou] from comment #13) > > Our primary target for testing should probably be Windows 7 or newer (I > > don't think XP supported MTP, but I'm not sure). I know I could never get > > MTP to work (with my nexus 4/5) on my laptop when it was running 12.04, but > > it seems to work fine under 14.04. > > Yeah, I agree. Get MTP working on Windows 7 should be our primary goal (I > believe that Windows XP doesn't support MTP). Just like I mentioned in the > meeting this morning, Alphan will start testing on Windows 7 and we'll see > what else we can do to complete this feature. Correct some information. Actually Windows XP SP2 can support MTP, which requires install Windows Media Player 10 or higher. I have ever used MTP on Windows XP before.

(In reply to Alphan Chen[:Alphan] from comment #14) > (In reply to Eric Chou [:ericchou] [:echou] from comment #13) > > > Our primary target for testing should probably be Windows 7 or newer (I > > > don't think XP supported MTP, but I'm not sure). I know I could never get > > > MTP to work (with my nexus 4/5) on my laptop when it was running 12.04, but > > > it seems to work fine under 14.04. > > > > Yeah, I agree. Get MTP working on Windows 7 should be our primary goal (I > > believe that Windows XP doesn't support MTP). Just like I mentioned in the > > meeting this morning, Alphan will start testing on Windows 7 and we'll see > > what else we can do to complete this feature. > > Correct some information. > Actually Windows XP SP2 can support MTP, which requires install Windows > Media Player 10 or higher. > I have ever used MTP on Windows XP before. Good to know. Thanks.

Comment on attachment 8450482 [details] [diff] [review] WIP - Able to open files via MTP Review of attachment 8450482 [details] [diff] [review]: ----------------------------------------------------------------- Hi Dave, Overall it looks good to me. Please answer my questions and address nits mentioned below. Thanks! ::: dom/system/gonk/AutoMounter.cpp @@ +404,5 @@ > sAutoMounter->UpdateState(); > } > } > > +void AutoMounter::StartMtpServer() super-nit: please wrap just like how we did for AutoMounter::UpdateState(). @@ +415,5 @@ > + sMozMtpServer = new MozMtpServer(); > + sMozMtpServer->Run(); > +} > + > +void AutoMounter::StopMtpServer() ditto @@ +464,5 @@ > } > > bool umsAvail = false; > bool umsEnabled = false; > + bool mtpAvail = false; Changes below looks good to me, not 100% sure I didn't miss anything, though. Since you are an expert of AutoMounter, I'm happy as long as you are ok with this. ;) ::: dom/system/gonk/MozMtpCommon.h @@ +6,5 @@ > + > +#ifndef mozilla_system_mozmtpcommon_h__ > +#define mozilla_system_mozmtpcommon_h__ > + > +#ifdef MOZ_WIDGET_GONK Since MozMtpCommon.h has been placed under dom/system/gonk which should only be built for Firefox OS, maybe we could just leverage from that so we don't need to do #ifdef MOZ_WIDGET_GONK. Does that make sense? ::: dom/system/gonk/MozMtpDatabase.cpp @@ +23,5 @@ > +} > + > +BEGIN_MTP_NAMESPACE > + > +const char *ObjectPropertyAsStr(MtpObjectProperty aProperty) Since this function is only used in this file, how about making it static? @@ +83,5 @@ > +void > +MozMtpDatabase::RemoveEntry(MtpObjectHandle aHandle) > +{ > + if (aHandle > 0 && aHandle < mDb.Length()) { > + mDb[aHandle].forget(); Question: why not doing |mDb[aHandle] = nullptr|? Moreover, mDb.RemoveElement() seems to be more reasonable to me since AddEntry does mDb.AppendElement(), although I know it would make mDb become more difficult to maintain, so it's not a must-have at least in this patch. @@ +114,5 @@ > + PRDirEntry* dirEntry; > + while ((dirEntry = PR_ReadDir(dir, PR_SKIP_BOTH))) { > + nsPrintfCString filename("%s/%s", aDir, dirEntry->name); > + PRFileInfo64 fileInfo; > + if (PR_GetFileInfo64(filename.get(), &fileInfo) != PR_SUCCESS ) { super-nit: please remove the trailing space of 'PR_SUCCESS' @@ +139,5 @@ > + entry->mObjectFormat = MTP_FORMAT_ASSOCIATION; > + entry->mObjectSize = 0; > + AddEntry(entry); > + ParseDirectory(filename.get(), entry->mHandle); > + } Should we print error log if fileInfo.type equals PR_FILE_OTHER? If that should never happen then maybe adding a MOZ_ASSERT would be a good idea. @@ +150,5 @@ > + //TODO: Add an assert re thread being run on > + > + PRFileInfo fileInfo; > + > + if (PR_GetFileInfo(aDir, &fileInfo) != PR_SUCCESS ) { super-nit: please remove the trailing space of 'PR_SUCCESS' @@ +400,5 @@ > + > + result.Clear(); > + > + switch (aMatchType) { > + nit: trailing whitespaces @@ +404,5 @@ > + > + case MatchAll: > + for (entryIdx = 0; entryIdx < numEntries; entryIdx++) { > + if (mDb[entryIdx]) { > + result.AppendElement(mDb[entryIdx]); nit: trailing whitespace @@ +442,5 @@ > + case MatchHandleFormat: > + for (entryIdx = 0; entryIdx < numEntries; entryIdx++) { > + entry = mDb[entryIdx]; > + if (entry->mHandle == aMatchField1) { > + if (entry && entry->mObjectFormat == aMatchField2) { The first condition seems to be unnecessary to me since |entry->mHandle| has passed. @@ +443,5 @@ > + for (entryIdx = 0; entryIdx < numEntries; entryIdx++) { > + entry = mDb[entryIdx]; > + if (entry->mHandle == aMatchField1) { > + if (entry && entry->mObjectFormat == aMatchField2) { > + result.AppendElement(entry); nit: trailing whitespace ::: dom/system/gonk/MozMtpDatabase.h @@ +21,5 @@ > + > +class MozMtpDatabase : public MtpDatabase > +{ > +public: > + MozMtpDatabase(const char *dir); nit: 'a'Dir @@ +137,5 @@ > + MatchParentFormat, > + }; > + > + > + void AddEntry(DbEntry *entry); nit: 'a'Entry @@ +141,5 @@ > + void AddEntry(DbEntry *entry); > + mozilla::TemporaryRef<DbEntry> GetEntry(MtpObjectHandle aHandle); > + void RemoveEntry(MtpObjectHandle aHandle); > + void QueryEntries(MatchType aMatchType, uint32_t aMatchField1, > + uint32_t aMatchField2, DbArray& result); nit: 'a'Result @@ +143,5 @@ > + void RemoveEntry(MtpObjectHandle aHandle); > + void QueryEntries(MatchType aMatchType, uint32_t aMatchField1, > + uint32_t aMatchField2, DbArray& result); > + > + nsCString BaseName(const nsCString& path); nit: 'a'Path @@ +152,5 @@ > + return mDb.Length(); > + } > + > + void ParseDirectory(const char *aDir, MtpObjectHandle aParent); > + void ReadVolume(const char *volumeName, const char *aDir); nit: 'a'VolumeName ::: dom/system/gonk/MozMtpServer.cpp @@ +95,5 @@ > +} > + > +void > +MozMtpServer::Run() > +{ nit: Should we check if mServerThread is running? I know we have a check in AutoMounter::StartMtpServer() so I'm also ok if you don't want to add a protection here. I asked just because want to make sure you're aware of it.

(In reply to Eric Chou [:ericchou] [:echou] from comment #16) > Comment on attachment 8450482 [details] [diff] [review] > WIP - Able to open files via MTP > > Review of attachment 8450482 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi Dave, > > Overall it looks good to me. Please answer my questions and address nits > mentioned below. Thanks! > > @@ +83,5 @@ > > +void > > +MozMtpDatabase::RemoveEntry(MtpObjectHandle aHandle) > > +{ > > + if (aHandle > 0 && aHandle < mDb.Length()) { > > + mDb[aHandle].forget(); > > Question: why not doing |mDb[aHandle] = nullptr|? Moreover, > mDb.RemoveElement() seems to be more reasonable to me since AddEntry does > mDb.AppendElement(), although I know it would make mDb become more difficult > to maintain, so it's not a must-have at least in this patch. Assigning nullptr is essentially the same thing, and probably clearer. We can't really call RemoveElement since we use the handle as an index into the array. We can't change the handle for objects between sessionBegin/End. > @@ +139,5 @@ > > + entry->mObjectFormat = MTP_FORMAT_ASSOCIATION; > > + entry->mObjectSize = 0; > > + AddEntry(entry); > > + ParseDirectory(filename.get(), entry->mHandle); > > + } > > Should we print error log if fileInfo.type equals PR_FILE_OTHER? If that > should never happen then maybe adding a MOZ_ASSERT would be a good idea. We definitely shouldn't assert, since the contents of the filesystem are something beyond our control. PR_FILE_OTHER covers things like symlinks, special files (dev nodes) If the FS is vfat, then there can't be any of those. If the FS is sdcard internal (like on nexus4/5) then its a special fuse mounted FS which won't let you create those. Even if we could create them, I think we'd just want to ignore them (without a warning). If we had a reason to create something, I think the warning would just wind up being log-spam. > @@ +442,5 @@ > > + case MatchHandleFormat: > > + for (entryIdx = 0; entryIdx < numEntries; entryIdx++) { > > + entry = mDb[entryIdx]; > > + if (entry->mHandle == aMatchField1) { > > + if (entry && entry->mObjectFormat == aMatchField2) { > > The first condition seems to be unnecessary to me since |entry->mHandle| has > passed. Oops - the check for entry needs to be on the first if. mDb[0] is always nullptr, and removed entries can be nullptr. I could have changed the for loops to be from 1, but I decided to leave them from 0 to make sure we're testing for entry being non-null. > ::: dom/system/gonk/MozMtpServer.cpp > @@ +95,5 @@ > > +} > > + > > +void > > +MozMtpServer::Run() > > +{ > > nit: Should we check if mServerThread is running? I know we have a check in > AutoMounter::StartMtpServer() so I'm also ok if you don't want to add a > protection here. I asked just because want to make sure you're aware of it. I think an ASSERT is probably more appropriate. We shouldn't be getting success and a null ptr back from NS_NewNamedThread.

I address all of the nits (I think) here and added comments to other questions.

Comment on attachment 8453279 [details] [diff] [review] WIP - Able to open files via MTP v2 Review of attachment 8453279 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. Thanks! ::: dom/system/gonk/MozMtpDatabase.cpp @@ +551,5 @@ > + aPacket.putUInt32(numEntries); > + for (entryIdx = 0; entryIdx < numEntries; entryIdx++) { > + RefPtr<DbEntry> entry = result[entryIdx]; > + > + for (size_t propertyIdx = 0; propertyIdx < numObjectProperties; propertyIdx++ ) { super-nit: please remove the blank around "++" @@ +589,5 @@ > + break; > + > + case MTP_PROPERTY_DATE_MODIFIED: > + { > + aPacket.putUInt16(MTP_TYPE_STR); nit: two spaces should be used as indentation. In addition, I prefer not using "{ }" to wrap the implementation, just like what we do for case MTP_PROPERTY_DATE_ADDED below.

> > > @@ +83,5 @@ > > > +void > > > +MozMtpDatabase::RemoveEntry(MtpObjectHandle aHandle) > > > +{ > > > + if (aHandle > 0 && aHandle < mDb.Length()) { > > > + mDb[aHandle].forget(); > > > > Question: why not doing |mDb[aHandle] = nullptr|? Moreover, > > mDb.RemoveElement() seems to be more reasonable to me since AddEntry does > > mDb.AppendElement(), although I know it would make mDb become more difficult > > to maintain, so it's not a must-have at least in this patch. > > Assigning nullptr is essentially the same thing, and probably clearer. > > We can't really call RemoveElement since we use the handle as an index into > the array. We can't change the handle for objects between sessionBegin/End. > Yeah I noticed that. If we want to use RemoveElement(), we will not be able to use mDb.Length() to assign the next handle. Let's keep it as-is. > > @@ +139,5 @@ > > > + entry->mObjectFormat = MTP_FORMAT_ASSOCIATION; > > > + entry->mObjectSize = 0; > > > + AddEntry(entry); > > > + ParseDirectory(filename.get(), entry->mHandle); > > > + } > > > > Should we print error log if fileInfo.type equals PR_FILE_OTHER? If that > > should never happen then maybe adding a MOZ_ASSERT would be a good idea. > > We definitely shouldn't assert, since the contents of the filesystem are > something beyond our control. > > PR_FILE_OTHER covers things like symlinks, special files (dev nodes) > If the FS is vfat, then there can't be any of those. > If the FS is sdcard internal (like on nexus4/5) then its a special fuse > mounted FS which won't let you create those. > Even if we could create them, I think we'd just want to ignore them (without > a warning). If we had a reason to create something, I think the warning > would just wind up being log-spam. > ok, thanks for explanation! > > ::: dom/system/gonk/MozMtpServer.cpp > > @@ +95,5 @@ > > > +} > > > + > > > +void > > > +MozMtpServer::Run() > > > +{ > > > > nit: Should we check if mServerThread is running? I know we have a check in > > AutoMounter::StartMtpServer() so I'm also ok if you don't want to add a > > protection here. I asked just because want to make sure you're aware of it. > > I think an ASSERT is probably more appropriate. We shouldn't be getting > success and a null ptr back from NS_NewNamedThread. Make sense to me. Thanks!

Tested basic file operations by using my Flame with the patch. The host is Ubuntu 13.04. The result is listed below: Item Result ======================================================================================================= Browse file system ok Open a file Unable to open .jpg/.png but able to open .pdf (see comment 7). Same error is observed when using my Moto G so it shouldn't be our problem. Create a folder ok Rename a file / folder failed. (libmtp error: could not get property description.) Delete a file / folder failed. (libmtp error: could not delete object.) Copy a file / folder (host -> device) ok Copy a file / folder (device -> host) ok Copy a file / folder (device -> device) failed. (Operation not supported by backend) Move a file / folder (device -> device) failed. (Operation not supported by backend) Other issues: 1. The title shown on host's file explorer is 'Unnamed Device' 2. When I select a file, the file size of selected file shown on the host is always 0 bytes.

> Rename a file / folder failed. (libmtp error: could not > get property description.) > Delete a file / folder failed. (libmtp error: could not > delete object.) > 2. When I select a file, the file size of selected file shown on the host is > always 0 bytes. I have opened 3 issues (bug 1036861, bug 1036862, bug 1036863) for tracking problems mentioned above. Assigned to myself first since Alphan will take a look of why our MTP implementation doesn't even work on Windows 7. > 1. The title shown on host's file explorer is 'Unnamed Device' The good news is that the same problem can be seen when using my Moto G. Therefore this doesn't seem to be an issue.

On windows 7, we can see a portable device called flame. There is a volume called "firefox os" when entering this portable device. However, we cannot see any file when entering "firefox os" volume. After checking, os send the following command and get error response in the last command. OpenSession -> GetDeviceInfo -> GetObjectPropsSupported (MTP) -> GetStorageIDs-> GetStorageInfo -> GetObjectHandles -> GetObjectPropDesc (MTP) -> GetObjectPropList (Get error response at this command)

Here is the log of ubuntu and windows7. (Ubuntu successful case) V/MtpServer( 293): GetObjectPropList 5 format: NONE property: UNKNOWN group: 0 depth: 0 I/MozMtp ( 293): getObjectPropertyList: Handle: 0x00000005 Format: 0x00000000 aProperty: 0xffffffff aGroupCode: 0 aDepth 0 (NOT SUPPORTED) (Windows7 case) V/MtpServer( 293): GetObjectPropList 1 format: NONE property: MTP_PROPERTY_OBJECT_FORMAT group: 0 depth: 0 I/MozMtp ( 293): getObjectPropertyList: Handle: 0x00000001 Format: 0x00000000 aProperty: 0x0000dc02 aGroupCode: 0 aDepth 0 (NOT SUPPORTED) E/MozMtp ( 293): getObjectPropertyList: Unrecognized property code: 0

Comment on attachment 8453279 [details] [diff] [review] WIP - Able to open files via MTP v2 Review of attachment 8453279 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/MozMtpDatabase.cpp @@ +540,5 @@ > + numObjectProperties = MOZ_ARRAY_LENGTH(sSupportedObjectProperties); > + objectPropertyList = sSupportedObjectProperties; > + } else { > + // return property indicated by aProperty > + numObjectProperties = 1; add the following can fix windows 7 error. objectProperty = aProperty;

(In reply to Eric Chou [:ericchou] [:echou] from comment #19) > Comment on attachment 8453279 [details] [diff] [review] > WIP - Able to open files via MTP v2 > > Review of attachment 8453279 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with nits addressed. Thanks! ...snip... > @@ +589,5 @@ > > + break; > > + > > + case MTP_PROPERTY_DATE_MODIFIED: > > + { > > + aPacket.putUInt16(MTP_TYPE_STR); > > nit: two spaces should be used as indentation. In addition, I prefer not > using "{ }" to wrap the implementation, just like what we do for case > MTP_PROPERTY_DATE_ADDED below. I actually need braces here, so I went the other way and added them to both cases. Basically, anytime you declare variables in the case, you should use braces. Otherwise the PRExplodedTime in one case duplicates the one in the other case. It happened to compile for me because one was declared with braces, so the one without was actually creating a shadow version.

https://hg.mozilla.org/integration/b2g-inbound/rev/48c4d4a97a4b I added the fix that Alphan noticed in comment 25 as well. Thanks for finding that.

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/ea6b5deb2c51 for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=43643271&tree=B2g-Inbound

ok - it looks like the emulator toolchains on TBPL need to be updated to add all of the files from frameworks/av/media/mtp I'll need to find somebody who can help me figure out the process of getting that done.

This is the updated version of the patch that got backed out. Since it was backed out, I figured I'd attach it so everyone can still use it while we figure out how to get TBPL updated.

Update the current status on windows 7. After fix the problem comment 25 mentioned, now we got the following command without any error response. However, there is no any item when I click into "firefox os" folder. I'm still trying to figure out the problem. 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 finally got green try builds: https://tbpl.mozilla.org/?tree=Try&rev=325f13dde470 emulator-ics is sdk 15, so it needed some compile flags. I discovered I wasn't doing debug builds, so one of the asserts threw a compiler error. For emulator-kk I needed to add libmtp.so into the dependency chain in gonk-misc I'm just doing another try run with just adding libmtp.so and if that works then I'll land that change first and make sure it shows up on the mirrors and then go ahead and try relanding this bug.

Comment on attachment 8454750 [details] [diff] [review] Updated patch Review of attachment 8454750 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +261,5 @@ > AC_SUBST(MOZ_OMX_ENCODER) > AC_DEFINE(MOZ_OMX_ENCODER) > ;; > 19) > + GONK_INCLUDES="-I$gonkdir/frameworks/native/include -I$gonkdir/frameworks/av/include -I$gonkdir/frameworks/av/include/media -I$gonkdir/frameworks/av/include/camera -I$gonkdir/frameworks/native/include/media/openmax -I$gonkdir/frameworks/av/media/libstagefright/include -I$gonkdir/frameworks/av/media/mtp" Please don't add new paths to GONK_INCLUDES. Add the paths to the appropriate moz.build where necessary. We actually want to reduce the size of GONK_INCLUDES but it's a bit tricky.

Now touches the CLOBBER file since we changed configure.in Added include path for sdk 15 (i.e. emulator-ics)

(In reply to Michael Wu [:mwu] from comment #33) > Comment on attachment 8454750 [details] [diff] [review] > Updated patch > > Review of attachment 8454750 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: configure.in > @@ +261,5 @@ > > AC_SUBST(MOZ_OMX_ENCODER) > > AC_DEFINE(MOZ_OMX_ENCODER) > > ;; > > 19) > > + GONK_INCLUDES="-I$gonkdir/frameworks/native/include -I$gonkdir/frameworks/av/include -I$gonkdir/frameworks/av/include/media -I$gonkdir/frameworks/av/include/camera -I$gonkdir/frameworks/native/include/media/openmax -I$gonkdir/frameworks/av/media/libstagefright/include -I$gonkdir/frameworks/av/media/mtp" > > Please don't add new paths to GONK_INCLUDES. Add the paths to the > appropriate moz.build where necessary. We actually want to reduce the size > of GONK_INCLUDES but it's a bit tricky. Is that going to work? We need different paths for different SDK versions. Does moz.build have access to the SDK version?

Yeah. See widget/gonk/libdisplay/moz.build for one example.

Removed CLOBBER Removed include search paths from configure.in and added to moz.build instead Try: https://tbpl.mozilla.org/?tree=Try&rev=78307754c967

Finally - got a green try run: https://tbpl.mozilla.org/?tree=Try&rev=e39cb6e2863e