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

VERIFIED FIXED in Firefox 34, Firefox OS v2.1

Status

Firefox OS
MTP/UMS
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Alison Shiue, Assigned: edenchuang)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

(Whiteboard: [ft:Peripherals])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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)
(Reporter)

Comment 1

3 years ago
[Blocking Requested - why for this release]:
functional issue
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Storage]
Adding qawanted for branch checks.
Keywords: qawanted

Comment 3

3 years ago
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]
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?][COM=Storage] → [QAnalyst-Triage+][COM=Storage]
Flags: needinfo?(jmitchell)
(Assignee)

Comment 5

3 years ago
Created attachment 8500332 [details] [diff] [review]
Handle "modified" event in MozMtpServer and MozMtpDatabase

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+
(Assignee)

Comment 7

3 years ago
Created attachment 8501487 [details] [diff] [review]
Bug1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer

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)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1074600
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)
(Assignee)

Comment 10

3 years ago
Created attachment 8501540 [details] [diff] [review]
Bug 1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer.

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+
(Assignee)

Comment 12

3 years ago
Created attachment 8501565 [details] [diff] [review]
Bug1074604: Handle file "modified" event in MozMtpDatabase and MozMtpServer. r=dhylands

Tryserver results fine.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=772478498578
Attachment #8501540 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

3 years ago
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/integration/b2g-inbound/rev/abff1c80ccea
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/abff1c80ccea
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
(Reporter)

Updated

3 years ago
Component: General → MTP/UMS
(Reporter)

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+][COM=Storage] → [QAnalyst-Triage+][COM=MTP/UMS]
(Reporter)

Updated

3 years ago
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)

Updated

3 years ago
status-b2g-v2.2: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb2c26b10c0b
status-b2g-v2.1: affected → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
(Reporter)

Comment 18

3 years ago
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
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.