Closed Bug 827311 Opened 7 years ago Closed 7 years ago

[Gallery] Video recording is not appearing if you jump from camera to the gallery app

Categories

(Firefox OS Graveyard :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 wontfix, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: tchung, Assigned: dougt)

References

Details

(Keywords: regression, Whiteboard: smoketest)

Attachments

(1 file)

The gallery is not showing a previously recorded videos in the gallery until you reboot the phone.  i think this regressed.  

REpro:
1) install 20130106070203 nightly unagi build
2) launch camera, and pick video recording.  record a video
3) stop the video recording, and launch gallery app
4) Verify the video you recorded is not there
5) now reboot the phone, and launch gallery.  recorded video appears now

Expected:
- video recording appears in gallery without rebotting 

actual;
- video recording doesnt appear in gallery until you reboot the phone
it looks like this only happens if you are launching directly in web activity from the camera app to the gallery app.   the recorded video doesnt appear then.  however, if you kill the gallery app and restart it, it will show the video.
Summary: [Gallery] Video recording is not appearing in gallery until after rebooting → [Gallery] Video recording is not appearing if you jump from camera to the gallery app
Gallery isnt seeing a device storage notification for recorded videos (it is seeing the creation of the dummy.3gp files)

It doesnt matter about the web activity, I can reproduce as long as the gallery was open prior to recording the video
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
I wonder if this is the same double-localized bug that was messing up the pick activity...

Taking it.
Assignee: nobody → dflanagan
(In reply to Tony Chung [:tchung] from comment #1)
> it looks like this only happens if you are launching directly in web
> activity from the camera app to the gallery app.   the recorded video doesnt
> appear then.  however, if you kill the gallery app and restart it, it will
> show the video.

I was incorrect, my gallery app was running already in the background when i hit this.  So the repro steps again are:

REpro:
1) install 20130106070203 nightly unagi build
2) open the gallery app.
3) launch camera, and pick video recording.  record a video
4) stop the video recording, and open the gallery app
5) Verify the video you recorded is not there

Note that this works if gallery app is not opened.
So comments 2 and 4 mean that this bug has existed for some time now. The camera hardware is saving the video file without notifying device storage about it. So device storage can't notify the gallery app about the new file.

Cc'ing Doug and Mike.

I could swear I had filed a bug about this. I'm not seeing it now, but I'm pretty certain that Doug and Mike and I have discussed this.  How can we work around this, guys?

Ideally, Mike could just make the camera hardware work with device storage so this didn't happen.

That seems sort of unrealistic at this point though. Doug: if you could give me a DeviceStorage.touch() function that would allow me to generate a modified event on a named file, I could fix the bug with that.

If we don't have time to fix this in gecko, can you think of workarounds in Gaia?  
The best I can come up with is a real hack: After the camera saves a video to the file foo.3gpp, the camera app could create a file like foo.3gpp.HEY_GALLERY.  Then the gallery app would see that dummy file and know to look for foo.3gpp.
I have been looking into this, the VideoRecorder is notifying devicestorage

https://github.com/mozilla/mozilla-central/blob/master/dom/camera/GonkCameraControl.cpp#L915

Its getting lost somewhere between 

We do already create the dummy file and the gallery app recieves its notification for that, so it knows once it has seen X.dummy.3gp to look for X next time it gets in focus
Does not seems a Gaia bug.
Component: Gaia::Gallery → General
QA Contact: jhammink
(In reply to Dale Harvey (:daleharvey) from comment #6)
> I have been looking into this, the VideoRecorder is notifying devicestorage
> 
> https://github.com/mozilla/mozilla-central/blob/master/dom/camera/
> GonkCameraControl.cpp#L915
> 
> Its getting lost somewhere between 

I just confirmed that with Mike, too.

> 
> We do already create the dummy file and the gallery app recieves its
> notification for that, so it knows once it has seen X.dummy.3gp to look for
> X next time it gets in focus

Yes. That dummy file is written before the video is recorded, and I was thinking of creating another after the file was saved, but maybe it would be enough to handle it on mozvisiblitychange.  Yuck.
Doug,

Does this sound like a device-storage bug to you?  Mike says the camera hardware is sending a 'file-watcher-update' observer notification.  Could something have regressed in device storage so it isn't receiving that notification anymore?

I think that Vivien is right that this is not a gaia bug. We can probably hack around it in Gaia, but that's not really the right thing to do.  Want to take this from me?
Flags: needinfo?(doug.turner)
djf: I had to go back to an old folder full of patches to find this bug number; since it was such a pain to track down, I'm posting it here for reference: bug 817496 - the bug where the DeviceStorage notification of new videos was added.
Assignee: dflanagan → doug.turner
clearing needinfo.
Flags: needinfo?(doug.turner)
Attached patch patch v.1Splinter Review
Attachment #699670 - Flags: review?(bent.mozilla)
Comment on attachment 699670 [details] [diff] [review]
patch v.1

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

r=me if the one non-nit below is alright. I'm not sure I understand how it is ok though so I'll come chat with you about it.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +230,5 @@
> +  NS_DECL_NSIOBSERVER
> +
> +  static FileUpdateDispatcher* GetSingleton();
> + private:
> +  static nsRefPtr<FileUpdateDispatcher> gSingleton;

Please use StaticRefPtr here. (Also, 'g' means global, 's' is better for static members)

@@ +256,5 @@
> +
> +NS_IMETHODIMP
> +FileUpdateDispatcher::Observe(nsISupports *aSubject,
> +                                           const char *aTopic,
> +                                           const PRUnichar *aData)

Nit: indent is off here.

@@ +262,5 @@
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +
> +    DeviceStorageFile* file = static_cast<DeviceStorageFile*>(aSubject);
> +    if (!file || !file->mFile) {
> +      return NS_OK;

Nit: You should probably warn here.

@@ +326,5 @@
>  
>    DebugOnly<DeviceStorageTypeChecker*> typeChecker = DeviceStorageTypeChecker::CreateOrGet();
>    NS_ASSERTION(typeChecker, "DeviceStorageTypeChecker is null");
> +
> +  DebugOnly<FileUpdateDispatcher*> observer = FileUpdateDispatcher::GetSingleton();

So this means that a child process won't broadcast new file notifications unless that child process has also used DeviceStorage once. Is that ok?

::: dom/ipc/ContentParent.cpp
@@ +1965,5 @@
> +        return true;
> +    }
> +    nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(aType, file);
> +
> +

Nit: two newlines here.

@@ +1972,5 @@
> +        return false;
> +    }
> +    nsString data;
> +    CopyASCIItoUTF16(aReason, data);
> +    obs->NotifyObservers(dsf, "file-watcher-update", data.get());

Nit: You could just do:

  obs->NotifyObservers(dsf, "file-watcher-update", NS_ConvertASCIItoUTF16(aReason).get());

::: dom/ipc/ContentParent.h
@@ +301,5 @@
>                                   InfallibleTArray<nsString>* aRetvals);
>      virtual bool RecvAsyncMessage(const nsString& aMsg,
>                                    const ClonedMessageData& aData);
>  
> +    virtual bool RecvFilePathUpdateNotify(const nsString& aType, const nsString& aFilePath, const nsCString& aReason);

Nit: looks like this file has one arg per line

::: dom/ipc/PContent.ipdl
@@ +438,5 @@
>  
>      async AudioChannelRegisterType(AudioChannelType aType);
>      async AudioChannelUnregisterType(AudioChannelType aType);
>  
> +    async FilePathUpdateNotify(nsString type, nsString filepath, nsCString reasons);

Nit: 'reason', not 'reasons'?
Attachment #699670 - Flags: review?(bent.mozilla) → review+
Marking FIXED as this is landed on inbound and b2g18.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 833094
Blocks: 833095
Blocks: 837236
You need to log in before you can comment on or make changes to this bug.