Closed Bug 1074604 Opened 10 years ago Closed 10 years ago

[MTP][KK] PC could not sync video which is recorded by device when MTP enable and USB plugged in

Categories

(Firefox OS Graveyard :: MTP/UMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: ashiue, Assigned: edenchuang)

Details

(Whiteboard: [ft:Peripherals])

Attachments

(1 file, 3 obsolete files)

Gaia-Rev        13973ab50760d1e8bb773082163f0dff19d35a44
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/6e317e075d04
Build-ID        20140928160204
Version         34.0a2

STR:
1. USB plugged in and enable MTP
2. Device record an video

Expect result:
1. The video shows on PC

Actual result:
1. The video does not show on PC (only generate an image which has the same name as the video on PC)
[Blocking Requested - why for this release]:
functional issue
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Storage]
Adding qawanted for branch checks.
Keywords: qawanted
Triage: blocker.
Assignee: nobody → echuang
blocking-b2g: 2.1? → 2.1+
Whiteboard: [ft:Peripherals]
QA Contact: ckreinbring
The bug repros on Flame 2.2 and Flame 2.1 using shallow flash.
Actual result: With MTP enabled, no file appears on the PC after recording a video from the Camera app.

Flame 2.2
BuildID: 20141003070740
Gaia: a8a6eed2ba9d66239aac789b9ee4900f911c73cb
Gecko: 388e101e75c8
Platform Version: 35.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1
BuildID: 20141003074236
Gaia: 88f431fcf75f274d9db66b9d3c8af7b6dc29edb1
Gecko: 0272a82658ff
Platform Version: 34.0a2
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

--------------------------------------------------------------------------------------------------------

The bug cannot be reproed on Flame 2.0 due to the fact that it does not have MTP functionality.

BuildID: 20141003065235
Gaia: 092d2b7678774c8b0b06dca0e0a8119e9eafdec3
Gecko: 43e53f774179
Platform Version: 32.0
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [COM=Storage] → [QAnalyst-Triage?][COM=Storage]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?][COM=Storage] → [QAnalyst-Triage+][COM=Storage]
Flags: needinfo?(jmitchell)
Following is the logcat of the case to generate vedio VID_0006.3gp

1032 10-06 05:26:37.980  5490  5490 I MozMtp  : Observe: file-watcher-update: file DCIM/100MZLLA/tmp.3gp created
1033 10-06 05:26:37.980  5490  6019 I MozMtp  : FileWatcherUpdate: file /storage/sdcard/DCIM/100MZLLA/tmp.3gp created
1034 10-06 05:26:37.980  5490  6019 I MozMtp  : FileWatcherUpdate: About to call sendObjectAdded Handle 0x00000006 file /storage/sdcard/DCIM/100MZLLA/tmp.3gp
1035 10-06 05:26:37.980  5490  5490 I Gecko   : DSF (parent) file-watcher-update: mStorageType 'videos' mStorageName 'sdcard' mRootDir '' mPath 'DCIM/100MZLLA/tmp.3gp' mFile->GetPath '/storage/sdcard/DCIM/     100MZLLA/tmp.3gp'
1036 10-06 05:26:37.980  5490  5490 I MozMtp  : Observe: file-watcher-update: file DCIM/100MZLLA/tmp.3gp modified
1037 10-06 05:26:37.980  5490  6019 I MozMtp  : FileWatcherUpdate: file /storage/sdcard/DCIM/100MZLLA/tmp.3gp modified

...

1062 10-06 05:26:38.070  6029  6029 I Gecko   : DSF (child) DeviceStorageCreateFdParams: mStorageType 'videos' mStorageName 'sdcard' mRootDir '' mPath 'DCIM/100MZLLA/VID_0006.3gp' mFile->GetPath '/storage/     sdcard/DCIM/100MZLLA/VID_0006.3gp'
1063 10-06 05:26:38.070  5490  5490 I Gecko   : DSF (parent) file-watcher-update: mStorageType 'videos' mStorageName 'sdcard' mRootDir '' mPath 'DCIM/100MZLLA/tmp.3gp' mFile->GetPath '/storage/sdcard/DCIM/     100MZLLA/tmp.3gp'
1064 10-06 05:26:38.070  5490  5490 I MozMtp  : Observe: file-watcher-update: file DCIM/100MZLLA/tmp.3gp deleted
1065 10-06 05:26:38.070  5490  6019 I MozMtp  : FileWatcherUpdate: file /storage/sdcard/DCIM/100MZLLA/tmp.3gp deleted
8062 10-06 05:26:50.660  5490  5490 I Gecko   : DSF (parent) file-watcher-update: mStorageType 'videos' mStorageName 'sdcard' mRootDir '' mPath 'DCIM/100MZLLA/VID_0006.3gp' mFile->GetPath '/storage/sdcard/     DCIM/100MZLLA/VID_0006.3gp'

...

8063 10-06 05:26:50.660  5490  5490 I MozMtp  : Observe: file-watcher-update: file DCIM/100MZLLA/VID_0006.3gp modified
8064 10-06 05:26:50.660  5490  6019 I MozMtp  : FileWatcherUpdate: file /storage/sdcard/DCIM/100MZLLA/VID_0006.3gp modified


According to the logcat, we found that MtpServer never receive the create event for VID_0006.3gp. It seems that because VID_0006.3gp is renamed from tmp.3gp. MtpServer got the tmp.3gp "delete" event(see line 1065, and at line 1064, we can know the file VID_0006.3gp exists), but MtpServer does not know that is caused by file renaming. At last, Camera app will finish VID_0006.3gp generation, then send "modified" event(line 8064).

At the tmp.3gp delete point, MtpServer has no idea it is renamed to VID_0006.3gp, and cannot handle this proper. Fortunately, Camera will send "modified" event while finish VID_0006.3gp generation. If MozMtpServer receive a "modified" event but cannot find it file entry in database, MozMtpDatabase need to create the entry for the file. 

In fact, we need to update MozMtpDatabase properly for every "modified" event. Bug 1074600 is the case that we only create the entry in MozMtpDatabase but without update it while we receive "modified" event. It makes the file information be incomplete.
Attachment #8500332 - Flags: feedback?(dhylands)
Comment on attachment 8500332 [details] [diff] [review]
Handle "modified" event in MozMtpServer and MozMtpDatabase

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

I would also change endSendObject so that it only calls FileWatcherNotify(entry, "modified");

This means we can remove mBeginSendObjectCalled entirely.

So I think you're on the right track, but with these suggestions thing should work much better.

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +296,5 @@
>      RemoveEntry(entryHandle);
>      return;
>    }
> +
> +  if (aEventType.EqualsLiteral("modified")) {

After working on bug 1072535, I think that we should ignore "created" events, and treat "modified" like we used to treat created.

The mediadb ignores "created" and only processes "modified". Camera, Bluetooth and Download only generate modified events.

Within device storage, the created event is sent when the file is created (and has zero size). The modified event is sent when the data has been written. So I think it makes sense to not tell MTP about the file until we get the modified event.

So I would remove this and change "created" above (around line 270) to be "modified".

::: dom/system/gonk/MozMtpServer.cpp
@@ +107,5 @@
>        return NS_OK;
>      }
>  
>      NS_ConvertUTF16toUTF8 eventType(aData);
> +    if (!eventType.EqualsLiteral("created") && !eventType.EqualsLiteral("deleted") && !eventType.EqualsLiteral("modified")) {

Here. I think that we should do the same thing. Ignore created. Process modified and deleted.
Attachment #8500332 - Flags: feedback?(dhylands) → feedback+
Modified the first patch with suggestions in comment 6.
Replace "created" event handling with "modified" event handling.
Attachment #8500332 - Attachment is obsolete: true
Attachment #8501487 - Flags: review?(dhylands)
Comment on attachment 8501487 [details] [diff] [review]
Bug1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer

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

I don't understand why you're calling RemoveEntry when you get a modified notification.

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +271,4 @@
>      if (entryHandle != 0) {
> +      MTP_LOG("About to call sendObjectRemoved Handle 0x%08x file %s", entryHandle, filePath.get());
> +      aMtpServer->sendObjectRemoved(entryHandle);
> +      RemoveEntry(entryHandle);

Why are you removing the entry here?

::: dom/system/gonk/MozMtpServer.cpp
@@ +108,5 @@
>      }
>  
>      NS_ConvertUTF16toUTF8 eventType(aData);
> +    if (!eventType.EqualsLiteral("modified") && !eventType.EqualsLiteral("deleted")) {
> +      // Bug 1074604: Needn't handle "created" event, once file operation finished, it would trigger "modified" event

nit: wrap line at 80 characters
Attachment #8501487 - Flags: review?(dhylands)
Add comment for removing/readding entry for the existed file
Attachment #8501487 - Attachment is obsolete: true
Attachment #8501540 - Flags: review?(dhylands)
Comment on attachment 8501540 [details] [diff] [review]
Bug 1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer.

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

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +267,5 @@
>  
>    MtpObjectHandle entryHandle = FindEntryByPath(filePath);
>  
> +  if (aEventType.EqualsLiteral("modified")) {
> +    // To update the file information to the newest, we remove the entry for 

nit: trailing space

@@ +268,5 @@
>    MtpObjectHandle entryHandle = FindEntryByPath(filePath);
>  
> +  if (aEventType.EqualsLiteral("modified")) {
> +    // To update the file information to the newest, we remove the entry for 
> +    // the existed file, then re-add the entry for the file.

nit: existing

@@ +277,4 @@
>      }
>      entryHandle = CreateEntryForFile(filePath, aFile);
>      if (entryHandle == 0) {
> +      // creating entry for the file fail, we don't tell MTP

nit: s/fail/failed/
     s/we//

creating entry for the file failed, don't tell MTP

::: dom/system/gonk/MozMtpServer.cpp
@@ +108,5 @@
>      }
>  
>      NS_ConvertUTF16toUTF8 eventType(aData);
> +    if (!eventType.EqualsLiteral("modified") && !eventType.EqualsLiteral("deleted")) {
> +      // Bug 1074604: Needn't handle "created" event, once file operation 

nit: trailing space
Attachment #8501540 - Flags: review?(dhylands) → review+
Keywords: checkin-needed
Comment on attachment 8501565 [details] [diff] [review]
Bug1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer. r=dhylands

Approval Request Comment
[Feature/regressing bug #]: It is a bug of MTP implementation.
[User impact if declined]: when enable MTP, takes new video cannot appears on the connected host or file would not be completed.
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=772478498578
[Risks and why]: (Low risk) It is a correction.
[String/UUID change made/needed]: none
Attachment #8501565 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/abff1c80ccea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Component: General → MTP/UMS
QA Whiteboard: [QAnalyst-Triage+][COM=Storage] → [QAnalyst-Triage+][COM=MTP/UMS]
Flags: needinfo?(bbajaj)
Comment on attachment 8501565 [details] [diff] [review]
Bug1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer. r=dhylands

Alison,

can you help verify this fix once it lands on 2.1? Thanks!
Attachment #8501565 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bbajaj)
Verified on
[2.1]
Gaia-Rev        f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/75cd3a8fa031
Build-ID        20141012160202
Version         34.0a2

[2.2]
Gaia-Rev        3b81896f04a02697e615fa5390086bd5ecfed84f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/199fb29c3467
Build-ID        20141012160201
Version         35.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.