Do not request string descriptor if index is 0 #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is similar to a fix I made in 8397298 -- but impacts an API that only OpenMTP uses so I missed originally. Garmin devices have a 0 index for a couple string descriptors, which is valid but means it's not available - so requesting it is incorrect behavior.
|
awesome @CodyJung ! |
|
Hey @CodyJung, I've been trying to figure out a peculiar problem when sending objects to MTP devices: there are inconsistent delays of approximately 1-1.5 seconds between sending objects, regardless of their size. This issue is especially puzzling for small files, which are taking just as long to transfer as medium-sized files. I've checked the MTP specs and multiple other MTP implementations and followed their methods, but none of this has resolved the issue. Since you have better resources to understand the issue listed below, I'm hoping you could guide me regarding the problem. Test ImplementationI've implemented a test in Go to transfer files of various sizes. Here's the relevant code snippet: File: func TestAndroid(t *testing.T) {
dev, err := SelectDevice("")
if err != nil {
t.Fatal(err)
}
defer dev.Close()
//setDebug(dev)
info := DeviceInfo{}
err = dev.GetDeviceInfo(&info)
if err != nil {
t.Fatal("GetDeviceInfo failed:", err)
}
if !strings.Contains(info.MTPExtension, "android.com:") {
t.Log("no android extensions", info.MTPExtension)
return
}
if err = dev.Configure(); err != nil {
t.Fatal("Configure failed:", err)
}
sids := Uint32Array{}
err = dev.GetStorageIDs(&sids)
if err != nil {
t.Fatalf("GetStorageIDs failed: %v", err)
}
if len(sids.Values) == 0 {
t.Fatalf("No storages")
}
id := sids.Values[0]
////////////////////////////////
// START: TEST multiple bulk writes
////////////////////////////////
dataLengths := []int{1000000, 0, 1, 2, 10, 100, 10, 100, 500, 500, 65536, 65535, 65024, 64524, 65524, 1000000, 5000000, 5000500}
// Pre-generate data
var populatedData [][]byte
for _, length := range dataLengths {
data := make([]byte, length)
for i := range data {
data[i] = byte('0' + i%10)
}
populatedData = append(populatedData, data)
}
// Main test loop
for i, testSize := range dataLengths {
filename := fmt.Sprintf("test-mtp-file-%d-%d", time.Now().Unix(), rand.Intn(10000))
send := ObjectInfo{
StorageID: id,
ObjectFormat: OFC_Undefined,
ParentObject: 0xFFFFFFFF,
Filename: filename,
CompressedSize: uint32(testSize),
ModificationDate: time.Now(),
Keywords: "bla",
}
_, _, _, err := dev.SendObjectInfo(id, 0xFFFFFFFF, &send)
if err != nil {
t.Fatal("SendObjectInfo failed:", err)
} else {
buf := bytes.NewBuffer(populatedData[i])
startTime := time.Now()
err = dev.SendObject(buf, int64(len(populatedData[i])), EmptyProgressFunc)
duration := time.Since(startTime)
if err != nil {
t.Log("SendObject failed:", err)
} else {
transferSpeed := float64(testSize) / duration.Seconds() / 1024 / 1024 // MB/s
t.Logf("Elapsed time to upload `%s`: File size: %s, %.2f seconds, %d milliseconds, Transfer speed: %.2f MB/s\n",
filename,
humanize.Bytes(uint64(testSize)), // "github.com/dustin/go-humanize"
duration.Seconds(),
duration.Milliseconds(),
transferSpeed)
}
}
}
}Test ResultsHere are the logs from running this test: Key Observations
Analysis So FarI believe short packet sending could be a potential reason for this behavior:
Relevant code: Line 599 in 1c3302b if lastTransfer%packetSize == 0 {
// write a short packet just to be sure.
d.h.BulkTransfer(d.sendEP, buf[:0], 250) // timeout = 250ms
}
Relevant code: Line 322 in 1c3302b // Fetches one USB packet. The header is split off, and the remainder is returned.
// dest should be at least 512bytes.
func (d *Device) fetchPacket(dest []byte, header *usbBulkHeader) (rest []byte, bytesRead int, err error) {
n, err := d.h.BulkTransfer(d.fetchEP, dest[:d.fetchMaxPacketSize()], d.Timeout)
if n > 0 {
d.dataPrint(d.fetchEP, dest[:n])
}
// ...
}Could this be happening due to an internal buffer in the MTP device that, when filled with, say, 5 or 6 objects, takes time to clear, keeping the device busy until then?This is a significant performance bottleneck when transacting many tiny files. I would greatly appreciate any insights or guidance on resolving this issue. Thank you for your time and effort! |
|
So the good news is that I don't see the same issue when transferring data to my watch: I bumped up the number of small files to ~50, and never saw a file take more than 96 milliseconds. Same story for my Fujifilm camera: I'm wondering if there's something specific to the Android MTP implementation? |
|
I started looking through the AOSP source, and it seems like there might be an intentional pause (there's a 500ms x2 poll time, if I'm reading it right), but I also don't understand why it would start being an issue after a certain number of requests. I don't have an Android phone handy tonight, and honestly I'm not sure how much debugging I'll be able to do. |
|
@CodyJung Thanks for your input. What’s even stranger is that the 1-second delay sometimes doesn’t occur at all, although this is rare. However, after reconnecting the device, the delay issue started happening again. Not sure what nerve I touched that made the issue temporarily disappear. I will go through the AOSP code and see what I can find. Thanks a lot! 🔥🔥 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is similar to a fix I made in 8397298 -- but impacts an API that only OpenMTP uses so I missed originally.
Garmin devices have a 0 index for a couple string descriptors, which is valid but means it's not available - so requesting it is incorrect behavior.
Part of the fix for ganeshrvel/openmtp#153