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)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
B2G C4 (2jan on)
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(1 file, 3 obsolete files)

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
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
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.
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) {
Attached patch Adding a principal for a stream (obsolete) (deleted) — Splinter Review
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 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)
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.
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)
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+
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+
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
tracking-b2g18: ? → ---
Patch as checked in.
Assignee: lissyx+mozillians → paul
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
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?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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?
Assignee: paul → lissyx+mozillians
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 ?
It can't happen on desktop because we don't use |mozCameras| there. Note that we made the same fix for |getUserMedia|, though.
(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.
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 :/
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 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-
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
closing this one as we landed a patch, and I file a specific new bug for Unagi.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Blocks: 839981
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.
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
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: