Closed Bug 1058452 Opened 7 years ago Closed 7 years ago

Canvas can not copy video frames with CameraAPI.

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.1 S4 (12sep)
tracking-b2g backlog

People

(Reporter: GaryChen, Assigned: sotaro)

References

Details

(Whiteboard: [MemShrink])

Attachments

(3 files, 3 obsolete files)

Attached file test app
Canvas can not copy video frames with CameraAPI.
But if we use normal video files such as mpg4 or webm, canvas works fine.
-------------------------------------------------------------------
STR:
1. Please download test app[1].
2. Launch test-camera app
3. click "switcher" button to play video with real file.
4. canvas can draw video well.
5. click "switcher" button to play video with camera API.

expect:
canvas able to draw video as well.

actually:
canvas does not work.

[1]https://github.com/mpizza/webQRCode/archive/master.zip

-------------------------------------------------
Gaia      7e2165cbc81d32188a2a7cf0f4bac6a61c7f42fc
Gecko     https://hg.mozilla.org/mozilla-central/rev/18901d4f3edd
BuildID   20140825160203
Version   34.0a1
ro.build.version.incremental=96
ro.build.date=Fri May 23 11:17:40 CST 2014
B1TC400110G0
(In reply to GaryChen [:GaryChen][:PYChen][:陳柏宇] from comment #0)
> Created attachment 8478881 [details]
> test app
> 
> Canvas can not copy video frames with CameraAPI.
> But if we use normal video files such as mpg4 or webm, canvas works fine.
> -------------------------------------------------------------------
> STR:
> 1. Please download test app[1].
Sorry I missed some steps,
 1.2 Please put the app into gaia/dev_apps
 1.3 edit app.list file [2].
 1.4 flash into your device.
[2] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#apps.list
> 2. Launch test-camera app
> 3. click "switcher" button to play video with real file.
> 4. canvas can draw video well.
> 5. click "switcher" button to play video with camera API.
> 
> expect:
> canvas able to draw video as well.
> 
> actually:
> canvas does not work.
> 
> [1]https://github.com/mpizza/webQRCode/archive/master.zip
> 
> -------------------------------------------------
> Gaia      7e2165cbc81d32188a2a7cf0f4bac6a61c7f42fc
> Gecko     https://hg.mozilla.org/mozilla-central/rev/18901d4f3edd
> BuildID   20140825160203
> Version   34.0a1
> ro.build.version.incremental=96
> ro.build.date=Fri May 23 11:17:40 CST 2014
> B1TC400110G0
ni chiajung to check camera
Flags: needinfo?(chung)
Well, I highly suspect it is yet another ready state bug in VideoElement, I will check it later :)
Flags: needinfo?(chung)
I met this before, the problem is here:
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#5825

Canvas always uses this function to get the video frame, but the readystate is usually < 2 (NO_DATA, HAVE_META_DATA) for such MediaStream in video element.
(In reply to Chiajung Hung [:chiajung] from comment #4)
> I met this before, the problem is here:
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp#5825
> 
> Canvas always uses this function to get the video frame, but the readystate
> is usually < 2 (NO_DATA, HAVE_META_DATA) for such MediaStream in video
> element.

Steven, could you have someone to check the video readystate?
Flags: needinfo?(slee)
mozCamera calls NotifyBlockingChanged, [1], but MediaStreamGraphImpl::RunInStableState is not called. So that the runnable method in [1] is never executed to change the state.


[1] http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2771
Flags: needinfo?(slee)
(In reply to StevenLee[:slee] from comment #6)
> mozCamera calls NotifyBlockingChanged, [1], but
> MediaStreamGraphImpl::RunInStableState is not called. So that the runnable
> method in [1] is never executed to change the state.
> 
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/
> HTMLMediaElement.cpp#2771

Thanks for your investigation Steven, so could you let me know what should I process now? should I ni? anyone to check the problem you mentioned?

Thanks
Flags: needinfo?(slee)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #7)
> Thanks for your investigation Steven, so could you let me know what should I
> process now? should I ni? anyone to check the problem you mentioned?
> 
> Thanks
Hi roc, sotaro,

I'm not sure who is the right person to ask. The cause I found is on comment 6. Can someone help this issue?
Thanks.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(slee)
Flags: needinfo?(roc)
I take this bug. Since Bug 844248 was fixed. Camera preview stream does not go through MediaStreamGraph, it affect to this.

Current camera stream's connection is like the following.
https://github.com/sotaroikeda/firefox-diagrams/blob/master/dom/dom_camera_FirefoxOS_2_1.pdf?raw=true
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> I take this bug. Since Bug 844248 was fixed. Camera preview stream does not
> go through MediaStreamGraph, it affect to this.

It seems better to united to ediaStreamGraph again in near future.
CameraPreviewMediaStream calls MediaStreamListener's functions. It is called from camera control thread.
But HTMLMediaElement::StreamListener's implementation calls MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate().
It have to be called only from Media graph thread. So, they are not safe.
Dear Sotaro,

There is pre-study of a new feature blocked by this issue, so I'd like to know when a solution will be ready.

Sorry for urging you, any information from you is appreciated. 

Thanks.
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> CameraPreviewMediaStream calls MediaStreamListener's functions. It is called
> from camera control thread.

Can we queue up the notifications in CameraPreviewMediaStream and dispatch them in a NotifyPull callback running on the MSG thread?
Flags: needinfo?(roc)
Assignee: nobody → sotaro.ikeda.g
(In reply to Jack Liu from comment #12)
> Dear Sotaro,
> 
> There is pre-study of a new feature blocked by this issue, so I'd like to
> know when a solution will be ready.

I am going to provide a just works patch until next Monday or Tuesday.
(In reply to Sotaro Ikeda [:sotaro] from comment #14)

> I am going to provide a just works patch until next Monday or Tuesday.

Nice! then we can go ahead with your help, thanks a lot.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> 
> Can we queue up the notifications in CameraPreviewMediaStream and dispatch
> them in a NotifyPull callback running on the MSG thread?

This seems to mean that adding CameraPreviewMediaStream to MediaStreamGraph. Current CameraPreviewMediaStream is not added to MediaStreamGraph. But camera frames do not pass through the MediaStreamGraph for performance reasons. Then it seems to cause inconsistency problems like blocking/unblocking state. It seems simpler just to add special message queue to MediaStreamGraph/GraphDriver.
Component: General → Gaia::Camera
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> CameraPreviewMediaStream calls MediaStreamListener's functions. It is called
> from camera control thread.
> But HTMLMediaElement::StreamListener's implementation calls
> MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate().
> It have to be called only from Media graph thread. So, they are not safe.

Further, it causes memory leak. MediaStreamListener's implementation is HTMLMediaElement::StreamListener. It calls MediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate(). It adds an element of an array like the following.

>  void DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable)
>  {
>    *mPendingUpdateRunnables.AppendElement() = aRunnable;
>  }

But it is never consumed, because CameraPreviewMediaStream does not start MediaStreamGraph's thread.
[Blocking Requested - why for this release]: From Comment 17, CameraPreviewMediaStream causes memory leak.
blocking-b2g: --- → 2.0?
Whiteboard: [MemShrink]
At first, I tried to use MediaStreamGraph's thread. It seems to address the problem. But it seems high risk change and MediaStreamGraph's thread runs regularly. It regress performance and power usage.
This patch seems to work basically. But high risk and could regress performance.
Attachment #8485472 - Attachment is obsolete: true
Attached patch patch - Use FakeMediaStreamGraph (obsolete) — Splinter Review
Use fake MediaStreamGraph. Simple implementation. Low risk than previous patch.
Comment on attachment 8485473 [details] [diff] [review]
patch - Use FakeMediaStreamGraph

roc, can you give a feedback to the patch?
Attachment #8485473 - Flags: feedback?(roc)
Comment on attachment 8485473 [details] [diff] [review]
patch - Use FakeMediaStreamGraph

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

This is a huge hack. We shouldn't be using FakeMediaStreamGraph for anything other than unit tests.

However, given how camera preview streams aren't real streams in the graph and we don't want to change that on a branch, this does seem like the cleanest option.
Attachment #8485473 - Flags: feedback?(roc) → feedback+
Agree. This hack is only for MozCamra's camera preview stream.
Attachment #8485473 - Flags: review?(mhabicher)
Comment on attachment 8485473 [details] [diff] [review]
patch - Use FakeMediaStreamGraph

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

Looks good to me. r+ with the nits below fixed (and question answered).

::: dom/camera/CameraPreviewMediaStream.cpp
@@ +21,5 @@
>  
> +void
> +FakeMediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable)
> +{
> +  nsRefPtr<nsIRunnable> task = aRunnable;

Is the temporary variable necessary? You should just be able to stuff aRunnable into NS_DispatchToMainThread().

::: dom/camera/CameraPreviewMediaStream.h
@@ +12,5 @@
>  namespace mozilla {
>  
> +class FakeMediaStreamGraph : public MediaStreamGraph
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Image)

s/Image/FakeMediaStreamGraph/

@@ +19,5 @@
> +    : MediaStreamGraph()
> +  {
> +  }
> +
> +  virtual void DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable) MOZ_OVERRIDE;

nit: please break this up onto multiple lines.

@@ +24,5 @@
> +
> +protected:
> +  ~FakeMediaStreamGraph()
> +  {}
> +

nit: please remove the blank line.
Attachment #8485473 - Flags: review?(mhabicher) → review+
(In reply to Mike Habicher [:mikeh] from comment #26)
> Comment on attachment 8485473 [details] [diff] [review]
> patch - Use FakeMediaStreamGraph
> 
> Review of attachment 8485473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. r+ with the nits below fixed (and question answered).
> 
> ::: dom/camera/CameraPreviewMediaStream.cpp
> @@ +21,5 @@
> >  
> > +void
> > +FakeMediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable)
> > +{
> > +  nsRefPtr<nsIRunnable> task = aRunnable;
> 
> Is the temporary variable necessary? You should just be able to stuff
> aRunnable into NS_DispatchToMainThread().

Not necessary, I am going to remove it in a next patch.
(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> (In reply to Mike Habicher [:mikeh] from comment #26)
> > Comment on attachment 8485473 [details] [diff] [review]
> > patch - Use FakeMediaStreamGraph
> > 
> > Review of attachment 8485473 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me. r+ with the nits below fixed (and question answered).
> > 
> > ::: dom/camera/CameraPreviewMediaStream.cpp
> > @@ +21,5 @@
> > >  
> > > +void
> > > +FakeMediaStreamGraph::DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable)
> > > +{
> > > +  nsRefPtr<nsIRunnable> task = aRunnable;
> > 
> > Is the temporary variable necessary? You should just be able to stuff
> > aRunnable into NS_DispatchToMainThread().
> 
> Not necessary, I am going to remove it in a next patch.

Sorry, I forgot about it. It is necessary because already_AddRefed<nsIRunnable> is not automatically converted to nsIRunnable. There should be a local variable that correctly decrement the ref count.
Apply the comments. Carry "r=mikeh".
Attachment #8485473 - Attachment is obsolete: true
Attachment #8485899 - Flags: review+
Attachment #8485899 - Attachment is patch: true
Attachment #8485899 - Attachment mime type: text/x-patch → text/plain
Removing the following seems to cause the problem. It also initialize StreamBuffer in MediaStream.
> SetGraphImpl(MediaStreamGraph::GetInstance());
Fix the initialization.
Attachment #8485899 - Attachment is obsolete: true
Comment on attachment 8486027 [details] [diff] [review]
patch ver 3 - Use FakeMediaStreamGraph

Carry "r=mikeh".
Attachment #8486027 - Flags: review+
I don't think this is a blocker because this is using mozCameras directly outside of the scope of the camera app, which won't ever happen in a production environment.
blocking-b2g: 2.0? → backlog
https://hg.mozilla.org/mozilla-central/rev/286f6adfb567
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
Sirs, seems this patch doesn't work at all
(In reply to hanp from comment #39)
> Sirs, seems this patch doesn't work at all

Hi Hanp -

Could you elaborate more? it would be nice if you can explain why you said "this patch doesn't work at all" and also provide some logs.

Thanks

Vance
Flags: needinfo?(hanp)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #40)
> (In reply to hanp from comment #39)
> > Sirs, seems this patch doesn't work at all
> 
> Hi Hanp -
> 
> Could you elaborate more? it would be nice if you can explain why you said
> "this patch doesn't work at all" and also provide some logs.
> 
> Thanks
> 
> Vance

Hi, vance, I added some code in camera, did a simple work, that is:
1. add a canvas element in camera viewfinder
2. add some trigger to call canvas.getContext("2d").drawImage(video, 0, 0)
3. to show the result, hide the video and show the canvas
and the image of the video cannot be seen
Flags: needinfo?(hanp)
Sorry, vance, I think I made a mistake, wrong style makes the captured image is out of the screen.
Seems it works now. my mistake.
(In reply to hanp from comment #42)
> Sorry, vance, I think I made a mistake, wrong style makes the captured image
> is out of the screen.
> Seems it works now. my mistake.

No problem, glad to know it works on your end

Thanks

Vance
Depends on: 1069109
I wrote some test code, and found that this patch not works as wanted
The copied image is only part of the preview frame.

here's the test code. Could you please help us?
Flags: needinfo?(sotaro.ikeda.g)
Shimba, this bug is already closed. Can you create a new bug to handle your problem?  Comment 44 says that the part of the image could be copied. Therefore, it is a different problem than this bug.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(yaon)
(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> Shimba, this bug is already closed. Can you create a new bug to handle your
> problem?  Comment 44 says that the part of the image could be copied.
> Therefore, it is a different problem than this bug.

We have created a new bug#1072067.
Flags: needinfo?(yaon)
See Also: → 1071198
Dear sotaro,

This patch  cause this PR.
https://bugzilla.mozilla.org/show_bug.cgi?id=1094676

Please help check it. thanks.
Flags: needinfo?(sotaro.ikeda.g)
Hi, sotaro

we found your comment on Bug 1071198 says that need to apply the patch in Bug 1069109, which fixed a lot problems caused by this patch.
but we cannot apply the patch in bug 1069109, because it's for 2.2.
could you please help us to apply it on 2.0?
(In reply to hanp from comment #48)
> Hi, sotaro
> 
> we found your comment on Bug 1071198 says that need to apply the patch in
> Bug 1069109, which fixed a lot problems caused by this patch.
> but we cannot apply the patch in bug 1069109, because it's for 2.2.
> could you please help us to apply it on 2.0?

I created attachment 8522253 [details] [diff] [review] for b2gv2.0 in Bug 1069109. Can you confirm if it works?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #49)
> 
> I created attachment 8522253 [details] [diff] [review] for b2gv2.0 in Bug
> 1069109. Can you confirm if it works?

Hi, we checked this patch, and found the power consumption normalized
but it caused a new bug,
1. enter camera
2. press power to shutdown the screen
3. press power to light on the screen
4. slide to unlock the phone
and then, you'll see the camera preview screen flashed several times.

By the way, we also find music/video also has the power consumption problem.
Is this related with the FakeMediaStreamGraph patch?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to hanp from comment #50)
> By the way, we also find music/video also has the power consumption problem.
> Is this related with the FakeMediaStreamGraph patch?

I do not know about it. Your code is already differ from default b2g v2.0. There could be a some bugs that could affect like Bug 1048171.
Flags: needinfo?(sotaro.ikeda.g)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.