[Video][MediaDB] the dummy files from camera app are enumerated and processed by video app

RESOLVED FIXED

Status

Firefox OS
Gaia::Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: johnhu, Assigned: johnhu)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(1 attachment)

This bug is copied from a sub-issue of bug 899864. The STR to do it:
1. open video app
2. press home
3. open camera app and switch to camcorder mode
4. repetitive touch record button
5. press home and go back to video

The symptom:
1. scanning never stops.
2. the following error got from logcat:
E/GeckoConsole(13801): Content JS ERROR at app://video.gaiamobile.org/shared/js/mediadb.js:897 in anonymous: MediaDB.getFile: NotFoundError

The root cause of this bug is the error handling of video app is insufficient. It is caused by the video app catches the dummy video file created by camera app which is deleted after recording. And when paring metadata, the processFirstQueuedItem doesn't handle the error of mediadb.getFile()[1].

[1] https://github.com/mozilla-b2g/gaia/blob/ddf78455bbfcc2a2adba2e620443c19cbb325208/apps/video/js/metadata.js#L115
Assignee: nobody → johu
Blocks: 899864
blocking-b2g: --- → leo?
The video profile of camera app should be changed to 480p in Leo device. And I can reproduce this bug only on leo device.
Created attachment 793828 [details]
add code to ignore dummy files and handle getFile error.

1. handles the errror of mediadb.getFile().
2. Once error happened, continue to process the metadata queue.
Attachment #793828 - Flags: review?(dflanagan)
Another easy STR to reproduce this bug, but it may not be the valid user usage:
1. make lots video with camera app
2. open video app who scans the video files
3. use adb shell to delete all video files created by camera app.
Comment on attachment 793828 [details]
add code to ignore dummy files and handle getFile error.

John,

I don't understand how the dummy file being created by the camera is even being passed to the video app. The dummy filename has a '/.' in its path, and mediadb ignores files like those. So how is this happening?

If this is a known issue that occurs primarily because of the dummy files, I'd prefer to explicitly filter them out before we get to an error handler.  If we can't filter them in mediadb, then maybe you can filter them out in the video app.

It does still make sense to add the error handler function in your patch, but I'd prefer to see a more pro-active approach to preventing the error.

I'm not giving r- here, just resetting the flag.  And if you're too busy to investigate this further, then I'll give r+ if you add a comment explaining that it is the dummy files from camera that are causing the error.
Attachment #793828 - Flags: review?(dflanagan)
I didn't notice we have a ignore list before. I searched and found it at mediadb.

The following is the message dump from mediadb and I had add the filename at the end of message:
E/GeckoConsole( 1411): Content JS ERROR at app://video.gaiamobile.org/shared/js/mediadb.js:893 in anonymous: MediaDB.getFile: NotFoundError /sdcard/DCIM/105MZLLA/.VID_0021.3gp

The metadata parsing request is enumerated from mediadb at db.js line 83[1]. It is executed at the startup of video app. And dummy file seems pass the examination of ignoreName function of mediadb.

After reading the bug 838179, ignoreName function had changed to ignore only files whose folder name leading with dot[2]. That's the why dummy file passes the examination of ignoreName function. And according to bug 838179, this behavior is the correct way to do checking. I thought it will filter filename leading with dot out, at first.

I will change the patch to check it before getFile in video app.

[1] https://github.com/mozilla-b2g/gaia/blob/2f62e3aaa1e8421e0d8e536d2b61fa69fb1bd983/apps/video/js/db.js#L83
[2] https://github.com/mozilla-b2g/gaia/blob/2f62e3aaa1e8421e0d8e536d2b61fa69fb1bd983/shared/js/mediadb.js#L1238
Sorry, I think I am wrong. 

I will filter them out at mediadb, not video app. If the dummy file is still saved in mediadb, video app need to filter them out all the time. And the dummy file should totally be ignored. So, it shouldn't be saved into mediadb at first.
Summary: The error handling of video app is insufficient → [Video][MediaDB] the dummy files from camera app are enumerated and processed by video app
Comment on attachment 793828 [details]
add code to ignore dummy files and handle getFile error.

David, 

I kept the error handling code and put the filtering code in the ignoreName function of mediadb. It uses regexp to filter dummy file only. That keeps others who's name leading with dot in the original behavior.
Attachment #793828 - Attachment description: add code to handle getFile error. → add code to ignore dummy files and handle getFile error.
Attachment #793828 - Flags: review?(dflanagan)
No longer blocks: 899864
Blocks: 899864
Comment on attachment 793828 [details]
add code to ignore dummy files and handle getFile error.

John,

I had forgotten about bug 838179, and misread the code in ignoreName(). I never agreed with that bug and didn't like the fix, but I guess I'm the one who landed the patch...

I don't like adding a special case like this to mediadb, but am giving r+ because I know this is needed for a leo+ bug.  

Ideally, the regexp would be passed in as a property of the options object that is passed to the MediaDB constructor. Then we could customize just the video app's behavior and not have the regexp comparison required for all files in all apps.
Attachment #793828 - Flags: review?(dflanagan) → review+
Thanks David, 

I had opened a follow up bug 908531 about your suggestion. Once I clear my bug list. I can fix it.
merged to the master:
https://github.com/mozilla-b2g/gaia/commit/5e19a901799947aedfea9ec6e2ae0e826f86bded
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(In reply to David Flanagan [:djf] from comment #8)
> Comment on attachment 793828 [details]
> add code to ignore dummy files and handle getFile error.
> 
> John,
> 
> I had forgotten about bug 838179, and misread the code in ignoreName(). I
> never agreed with that bug and didn't like the fix, but I guess I'm the one
> who landed the patch...
> 
> I don't like adding a special case like this to mediadb, but am giving r+
> because I know this is needed for a leo+ bug. 

 [Triage comment] Given 899864 is no longer a blocker, don't think we would still need this.Please renominate with more information if you think we had a wrong assumption.
> 
> Ideally, the regexp would be passed in as a property of the options object
> that is passed to the MediaDB constructor. Then we could customize just the
> video app's behavior and not have the regexp comparison required for all
> files in all apps.
blocking-b2g: leo? → -
Duplicate of this bug: 940166
You need to log in before you can comment on or make changes to this bug.