Closed
Bug 832745
Opened 12 years ago
Closed 12 years ago
Cannot copy video frames from Camera to Canvas
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)
RESOLVED
FIXED
B2G C4 (2jan on)
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
lsblakk
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
Trying to write an application that targets video frames analysis (e.g., QR Code reader, things like this), I have:
- <video> element, with JS code that sets the source to the camera, .mozSrcObject = stream, which works
- <canvas> element, to which frames will be copied through Context2D.
The drawImage() call returns an exception:
Exception: [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: app://heart-rate.gaiamobile.org/js/heart-rate.js :: heartrate_computeFrame :: line 70" data: no]
It does not happens if the <video> targets a local WebM file.
Tracking the exception, I finally found the code path of the issue.
In |nsCanvasRenderingContext2DAzure::DrawImage()|, there is a call on
|CanvasImageCache::Lookup(element, mCanvasElement, &imgSize);| which fails, hence the next call will be on |nsLayoutUtils::SurfaceFromElement(element, sfeFlags);|.
It calls |nsLayoutUtils::SurfaceFromElement(nsHTMLVideoElement* aElement, uint32_t aSurfaceFlags)|, which will then fail on |nsCOMPtr<nsIPrincipal> principal = aElement->GetCurrentPrincipal();| with principal staying null.
Looking in details at GetCurrentPrincipal(), I had the following:
Breakpoint 10, nsHTMLMediaElement::GetCurrentPrincipal (this=0x41983e00) at /home/alex/codaz/B2G/gecko/content/html/content/src/nsHTMLMediaElement.cpp:3406
3406 if (mSrcStream) {
(gdb) print mSrcStream
$14 = {mRawPtr = 0x4462b700}
(gdb) n
3407 nsRefPtr<nsIPrincipal> principal = mSrcStream->GetPrincipal();
(gdb) print mSrcStream
$15 = {mRawPtr = 0x4462b700}
(gdb) print principal
$16 = {mRawPtr = 0xbe99b94e}
(gdb) n
3408 return principal.forget();
(gdb) print principal
$17 = {mRawPtr = 0x0}
(gdb) print mSrcStream
Cannot access memory at address 0x54
(gdb) info breakpoints
Num Type Disp Enb Address What
9 breakpoint keep y 0x406b7728 in nsHTMLMediaElement::GetCurrentPrincipal() at /home/alex/codaz/B2G/gecko/content/html/content/src/nsHTMLMediaElement.cpp:3407
breakpoint already hit 28 times
10 breakpoint keep y 0x406b7710 in nsHTMLMediaElement::GetCurrentPrincipal() at /home/alex/codaz/B2G/gecko/content/html/content/src/nsHTMLMediaElement.cpp:3406
breakpoint already hit 16 times
Assignee | ||
Comment 1•12 years ago
|
||
When the <video> has a "src" attribute that points to a WebM video, the code path in GetCurrentPrincipal() goes through the if (mDecoder) branch, and not if (mSrcStream), and succeeds.
No longer blocks: 787812
Assignee | ||
Comment 2•12 years ago
|
||
Could it be related to http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#2488, with the comment stating:
// XXX if we ever support capturing the output of a media element which is
// playing a stream, we'll need to add a CombineWithPrincipal call here.
Assignee | ||
Comment 3•12 years ago
|
||
This change seems to be enough to copy frames.
diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
index 79acad7..fcad957 100644
--- a/content/html/content/src/nsHTMLMediaElement.cpp
+++ b/content/html/content/src/nsHTMLMediaElement.cpp
@@ -2826,6 +2826,7 @@ void nsHTMLMediaElement::SetupSrcMediaStreamPlayback()
mSrcStream = mSrcAttrStream;
// XXX if we ever support capturing the output of a media element which is
// playing a stream, we'll need to add a CombineWithPrincipal call here.
+ mSrcStream->CombineWithPrincipal(NodePrincipal());
mSrcStreamListener = new StreamListener(this);
GetSrcMediaStream()->AddListener(mSrcStreamListener);
if (mPaused) {
Assignee | ||
Comment 4•12 years ago
|
||
Being able to copy frames from a stream, here the device's camera, is mandatory if we want to have applications like a barcode scanner.
Attachment #704508 -
Flags: review?(mhabicher)
Comment 5•12 years ago
|
||
Comment on attachment 704508 [details] [diff] [review]
Adding a principal for a stream
All I could do is rubber-stamp this. roc, what do you think?
Attachment #704508 -
Flags: review?(mhabicher) → review?(roc)
Updated•12 years ago
|
tracking-b2g18:
--- → ?
This patch isn't correct. The principal should have been set when we started capturing the stream. Something in or around DOMCameraPreview I guess.
You should get a principal from the document or window where the camera is first obtained, and then make sure that principal eventually gets set on the new nsDOMMediaStream.
Assignee | ||
Comment 8•12 years ago
|
||
Thanks to both roc and padenot feedback, I've implemented padenot's suggestion. As far as I can tell, it works on my Nexus S.
Attachment #704508 -
Attachment is obsolete: true
Attachment #704508 -
Flags: review?(roc)
Assignee | ||
Comment 9•12 years ago
|
||
You can find example usage at https://github.com/lissyx/heart-rate
Comment on attachment 706804 [details] [diff] [review]
Adding a principal for a stream when creating preview
Review of attachment 706804 [details] [diff] [review]:
-----------------------------------------------------------------
Let's land this!
Attachment #706804 -
Flags: review+
Assignee | ||
Comment 11•12 years ago
|
||
Re-attaching the same patch with a nice git commit, thus being ready to be landed.
Attachment #706804 -
Attachment is obsolete: true
Attachment #708057 -
Flags: review+
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Patch as checked in.
Updated•12 years ago
|
Assignee: lissyx+mozillians → paul
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 708057 [details] [diff] [review]
Adding a principal for a stream when creating preview, patch to be landed
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined:
Testing completed:
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:
Attachment #708057 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 708073 [details] [diff] [review]
Adding a principal for a stream when creating.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Unable to copy video frames to canvas, thus blocking many useful applications (barcode reader)
Testing completed: tested on nexus s with https://github.com/lissyx/heart-rate/ successfully
Risk to taking this patch (and alternatives if risky): low. alternative is to prevent authors from writing a whole class of applications
String or UUID changes made by this patch: N/A
Attachment #708073 -
Flags: approval-mozilla-b2g18?
Comment 16•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 17•12 years ago
|
||
Testing only with nexus s doesn't help assess the risk here for otoro/unagi and our ship devices. Can someone confirm that this doesn't cause regressions on those or otherwise make the risk/reward less desireable?
Updated•12 years ago
|
Assignee: paul → lissyx+mozillians
Comment 18•12 years ago
|
||
Alexandre, I tried on unagi with your patch (on the b2g18 branch) and I still have the bug.
BTW would it be possible to have your app somewhere so that we could install it easily without copying in a dev environment ?
Also, I don't understand exactly what is specific for B2G here; why doesn't the bug happen on Desktop Firefox ?
Comment 19•12 years ago
|
||
It can't happen on desktop because we don't use |mozCameras| there. Note that we made the same fix for |getUserMedia|, though.
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #18)
> Alexandre, I tried on unagi with your patch (on the b2g18 branch) and I
> still have the bug.
Maybe there is something else missing ; on Nexus S there is (still) bug 787812 which is said to be fixed but the fix disappeared ...
>
> BTW would it be possible to have your app somewhere so that we could install
> it easily without copying in a dev environment ?
I don't think it's usable enough to justify having it packaged.
>
> Also, I don't understand exactly what is specific for B2G here; why doesn't
> the bug happen on Desktop Firefox ?
Because, as paul said, on Desktop there is no mozCamera.
Assignee | ||
Comment 21•12 years ago
|
||
After rethinking about it under the shower and discussing about with paul, it comes to my mind that maybe you are missing bug 787812 fix: as far as I've found, the pixel format added in the patch attached to this bug is the one of Android preview stream. So maybe with some luck this is not Nexus S-specific, and that we also need it on your devices.
If it's not enough, then someone will have to dig through the calls to understand what's missing :/
Comment 22•12 years ago
|
||
Will happily try this today.
You should really put your app somewhere, I mean not necessarily packaged on a marketplace, but just a button to trigger an install from your existing manifest, as an hosted+appcache app. Github pages could be sufficient for this !
This would help drivers to test and understand the problem, and to make your patch into v1.
(mmm I can't add a needinfo when it's resolved/fixed grrr)
Comment 23•12 years ago
|
||
Comment on attachment 708073 [details] [diff] [review]
Adding a principal for a stream when creating.
Don't see where we have a requirement to uplift this - it looks like it should ride the trains since there's no product request and it's not a release blocker.
Attachment #708073 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Comment 24•12 years ago
|
||
Lukas> problem is : I reproduce the bug on Unagi, and the patch does not fix it (or I may have done something wrong).
So we should first find a patch that fixes this on Unagi and only then we might ask for an uplift.
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 25•12 years ago
|
||
closing this one as we landed a patch, and I file a specific new bug for Unagi.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
tracking-b2g18:
--- → ?
Comment 26•12 years ago
|
||
Thanks for splitting it out, I see we're tracking bug 839981 already so we'll get the Unagi fix there when it's ready for uplift. Nothing to track here in that case, again because this is not hardware we have to support in v1.
Comment 27•12 years ago
|
||
Lukas, actually the title of this bug was not accurate: we need this patch for Bug 839981 to work.
Summary: Cannot copy video frames from Camera to Canvas on Nexus S → Cannot copy video frames from Camera to Canvas
Updated•12 years ago
|
status-b2g18:
--- → affected
Comment 28•12 years ago
|
||
Comment on attachment 708073 [details] [diff] [review]
Adding a principal for a stream when creating.
So bug 839981 has already landed, at this point (post 3/15) we now require approval to uplift so marking this for uplift since it's required to make the other landed & tracked bug fix work.
Attachment #708073 -
Flags: approval-mozilla-b2g18- → approval-mozilla-b2g18+
Comment 29•12 years ago
|
||
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
You need to log in
before you can comment on or make changes to this bug.
Description
•