1058452 - Canvas can not copy video frames with CameraAPI.

Status: RESOLVED FIXED

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

Description

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: 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

Comment 1

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

(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

Comment 2

[Peter Chang[:pchang]]

ni chiajung to check camera

Comment 3

[Chiajung Hung [:chiajung]]

Well, I highly suspect it is yet another ready state bug in VideoElement, I will check it later :)

Comment 4

[Chiajung Hung [:chiajung]]

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.

Comment 5

[Peter Chang[:pchang]]

(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?

Comment 6

[StevenLee[: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

Comment 7

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 8

[StevenLee[:slee]]

(In reply to Vance Chen [:vchen][[e-mail]] 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.

Comment 9

[Sotaro Ikeda [:sotaro]]

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

Comment 10

[Sotaro Ikeda [:sotaro]]

(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.

Comment 11

[Sotaro Ikeda [:sotaro]]

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.

Comment 12

[Jack Liu]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 14

[Sotaro Ikeda [:sotaro]]

(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.

Comment 15

[Jack Liu]

(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.

Comment 16

[Sotaro Ikeda [:sotaro]]

(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.

Comment 17

[Sotaro Ikeda [:sotaro]]

(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.

Comment 18

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]: From Comment 17, CameraPreviewMediaStream causes memory leak.

Comment 19

[Sotaro Ikeda [:sotaro]]

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.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Use MediaStream's thread for callbacks

This patch seems to work basically. But high risk and could regress performance.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use FakeMediaStreamGraph

Use fake MediaStreamGraph. Simple implementation. Low risk than previous patch.

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8485473 [details] [diff] [review]
patch - Use FakeMediaStreamGraph

roc, can you give a feedback to the patch?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 24

[Sotaro Ikeda [:sotaro]]

Agree. This hack is only for MozCamra's camera preview stream.

Comment 25

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=30b2578431e2

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 27

[Sotaro Ikeda [:sotaro]]

(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.

Comment 28

[Sotaro Ikeda [:sotaro]]

(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.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 2 - Use FakeMediaStreamGraph

Apply the comments. Carry "r=mikeh".

Comment 30

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e2793c8ac7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e103d82e5fdd

https://tbpl.mozilla.org/php/getParsedLog.php?id=47621704&tree=Mozilla-Inbound

Comment 32

[Sotaro Ikeda [:sotaro]]

Removing the following seems to cause the problem. It also initialize StreamBuffer in MediaStream.
> SetGraphImpl(MediaStreamGraph::GetInstance());

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 3 - Use FakeMediaStreamGraph

Fix the initialization.

Comment 34

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8486027 [details] [diff] [review]
patch ver 3 - Use FakeMediaStreamGraph

Carry "r=mikeh".

Comment 35

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=6800fb3e770e

Comment 36

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/286f6adfb567

Comment 37

[Jason Smith [:jsmith]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/286f6adfb567

Comment 39

[hanp]

Sirs, seems this patch doesn't work at all

Comment 40

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 41

[hanp]

(In reply to Vance Chen [:vchen][[e-mail]] 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

Comment 42

[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.

Comment 43

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 44

[Simba Yao]

Attachment: 0001-canvas-test-code-patch.patch

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?

Comment 45

[Sotaro Ikeda [:sotaro]]

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.

Comment 46

[Simba Yao]

(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.

Comment 47

[Guoqiang.CHEN]

Dear sotaro,

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

Please help check it. thanks.

Comment 48

[hanp]

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?

Comment 49

[Sotaro Ikeda [:sotaro]]

(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?

Comment 50

[hanp]

(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?

Comment 51

[Sotaro Ikeda [:sotaro]]

(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.