Implement createObjectURL for MediaStreams

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks 1 bug)

Trunk
mozilla19
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 3 obsolete attachments)

1.31 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.23 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
18.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
975 bytes, patch
sicking
: review+
Details | Diff | Splinter Review
1.72 KB, patch
sicking
: review+
Details | Diff | Splinter Review
40.01 KB, patch
sicking
: review+
Details | Diff | Splinter Review
7.79 KB, patch
sicking
: review+
Details | Diff | Splinter Review
5.55 KB, patch
sicking
: review+
Details | Diff | Splinter Review
7.06 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.16 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.08 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
The main reason I want to switch the URL interface to use WebIDL is to get support for overloading createObjectURL.
The spec says createObjectURL(MediaStream) should return a Blob URI. That doesn't make complete sense since a Blob URI should refer to a particular immutable chunk of blob data, whereas a MediaStream keeps producing data indefinitely and definitely isn't equivalent to any immutable chunk of data. Using this URI as source for any resource loading except media elements does not make sense.

I think it would make more sense to have a new kind of URI, MediaStream URIs.
> The spec says createObjectURL(MediaStream) should return a Blob URI.
What spec defines that?
Comment on attachment 663350 [details] [diff] [review]
Part 1: Make the URL interface use WebIDL

It might be worth it to name the new class "mozilla::dom::URL", the new files URL.h and URL.cpp, and export the header into mozilla/dom.  That's the setup WebIDL bindings assume by default, though obviously you can annotate a different classname/header as you have.

>+++ b/dom/base/nsDOMURL.cpp
>+nsDOMURL::CreateObjectURL(nsISupports* aGlobal, nsIDOMBlob* aBlob,
>+  if (!window || !window->GetExtantDoc() || !aBlob) {

aBlob can't be null here (since the IDL doesn't allow it).  The only reason it's a pointer, not a reference, is that it's not a WebIDL interface yet...

>+  // The constructor and destructor (and mWindow) are not currently used,
>+  // since we currently have no way to instantiate an instance of the 'URL'
>+  // interface. Currently only the static methods are supported.

More on this below.

>+++ b/dom/base/nsPIDOMWindow.h
>+		{ 0x7b18e421, 0x2179, 0x4e24, \
>+		  { 0x96, 0x58, 0x26, 0x75, 0xa4, 0x37, 0xf3, 0x8f } }

Fix indent?

>+++ b/dom/bindings/Bindings.conf
>+'URL' : {

Do we want to follow File API and have URL.prototype be false?  I think we do for now.  So please add a 'concrete': False here.  That will incidentally mean you don't have to implement WrapObject or the construct and destructor or any of that stuff, since the binding code that would normally call into it won't be generated.

See also http://lists.w3.org/Archives/Public/public-script-coord/2012JulSep/0139.html

>+++ b/dom/webidl/URL.webidl
>+ * Copyright � 2012 W3C� (MIT, ERCIM, Keio), All Rights Reserved. W3C

The non-ASCII bits here got corrupted, looks like...

r=me with the above nits fixed.

Other than lack of support for static stuff, what pain points did you hit?
Attachment #663350 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #5)
> It might be worth it to name the new class "mozilla::dom::URL", the new
> files URL.h and URL.cpp, and export the header into mozilla/dom.  That's the
> setup WebIDL bindings assume by default, though obviously you can annotate a
> different classname/header as you have.

OK, I'll do that.

> >+++ b/dom/base/nsDOMURL.cpp
> >+nsDOMURL::CreateObjectURL(nsISupports* aGlobal, nsIDOMBlob* aBlob,
> >+  if (!window || !window->GetExtantDoc() || !aBlob) {
> 
> aBlob can't be null here (since the IDL doesn't allow it).  The only reason
> it's a pointer, not a reference, is that it's not a WebIDL interface yet...

OK, I'll take that out.

> >+++ b/dom/base/nsPIDOMWindow.h
> >+		{ 0x7b18e421, 0x2179, 0x4e24, \
> >+		  { 0x96, 0x58, 0x26, 0x75, 0xa4, 0x37, 0xf3, 0x8f } }
> 
> Fix indent?

OK

> >+++ b/dom/bindings/Bindings.conf
> >+'URL' : {
> 
> Do we want to follow File API and have URL.prototype be false?  I think we
> do for now.  So please add a 'concrete': False here.  That will incidentally
> mean you don't have to implement WrapObject or the construct and destructor
> or any of that stuff, since the binding code that would normally call into
> it won't be generated.

OK.

> Other than lack of support for static stuff, what pain points did you hit?

Nothing significant. It's good.
If I set 'concrete':False, I get this:

  File "z:\roc\mozilla-central\dom\bindings\Codegen.py", line 5712, in __init__
    cgthings.extend([CGDescriptor(x) for x in descriptors])
  File "z:\roc\mozilla-central\dom\bindings\Codegen.py", line 5250, in __init__
    properties = PropertyArrays(descriptor)
  File "z:\roc\mozilla-central\dom\bindings\Codegen.py", line 1094, in __init__
    self.methods = MethodDefiner(descriptor, "Methods", False)
  File "z:\roc\mozilla-central\dom\bindings\Codegen.py", line 976, in __init__
    assert not self.hasChromeOnly() and not self.hasNonChromeOnly()
AssertionError
BTW I have the rest of this bug implemented and working, just need to sort out the above issue and clean up my patches for submission. I refactored nsBlobProtocolHandler/nsBlobURI to be nsHostObjectProtocolHandler/nsBlobURI to handle both blob: URIs and mediastream: URIs.

(In reply to Masatoshi Kimura [:emk] from comment #4)
> > The spec says createObjectURL(MediaStream) should return a Blob URI.
> What spec defines that?

http://dev.w3.org/2011/webrtc/editor/getusermedia.html#url
> If I set 'concrete':False, I get this:

Is descriptor.interface.hasInterfaceObject() coming out false?  Or is this the descriptor.interface.hasInterfacePrototypeObject() case?  I'm not sure what exactly you have on line 976 in your patched Codegen.py.
        if static:
            if not descriptor.interface.hasInterfaceObject():
                # static methods go on the interface object
                assert not self.hasChromeOnly() and not self.hasNonChromeOnly()
        else:
            if not descriptor.interface.hasInterfacePrototypeObject():
                # non-static methods go on the interface prototype object
976:                assert not self.hasChromeOnly() and not self.hasNonChromeOnly()

Dumped values:
hasInterfaceObject:1 hasInterfacePrototypeObject:0 chromeOnly:1 nonChromeOnly:0

Not sure where the chromeOnly method is coming from. Investigating.
Oh, and this is the nonstatic methods case.
It's the QueryInterface method that's causing problems...
Comment on attachment 663862 [details] [diff] [review]
Part 0.5: Fix Codegen

r=me.  Good catch.  We should really work on restricting where we're adding that QueryInterface even more...
Attachment #663862 - Flags: review?(bzbarsky) → review+
Hmm, but now things are failing because CGDescriptor isn't generating the static method bindings. Looks like all method generation is under "if  descriptor.interface.hasInterfacePrototypeObject():" ...
That sounds like a bug in the patch for bug 763643...
Comment on attachment 663871 [details] [diff] [review]
Part 0.6: Fix Codegen some more

I think the right thing to do here is:

1)  In the isStatic() cases assert descriptor.interface.hasInterfaceObject.
2)  In the other case, test for the proto object.

So make it look like this:

                if m.isStatic():
                    assert descriptor.interface.hasInterfaceObject()
                    cgThings.append(CGStaticMethod(descriptor, m))
                elif descriptor.interface.hasInterfacePrototypeObject():
                    cgThings.append(CGSpecializedMethod(descriptor, m))
                    cgThings.append(CGMemberJITInfo(descriptor, m))
                    hasMethod = True

so we don't generate all that code for the non-static bits that we won't actually use.

r=me with that.
Attachment #663871 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #18)
>                 if m.isStatic():
>                     assert descriptor.interface.hasInterfaceObject()
>                     cgThings.append(CGStaticMethod(descriptor, m))
>                 elif descriptor.interface.hasInterfacePrototypeObject():
>                     cgThings.append(CGSpecializedMethod(descriptor, m))
>                     cgThings.append(CGMemberJITInfo(descriptor, m))
>                     hasMethod = True
> 
> so we don't generate all that code for the non-static bits that we won't
> actually use.

OK, but in what situations would we have nonstatic members and no interface prototype object? I guess if someone defined a WebIDL interface with nonstatic members but also made it 'concrete':False? Why would they do that? :-)
Posted patch Part 0.6 v2Splinter Review
I'm not confident hacking this code so requesting re-review.

In this version of the patch I moved the code to set has(Lenient)(Getter/Setter) into the non-static, hasInterfacePrototypeObject path.
Attachment #663871 - Attachment is obsolete: true
Attachment #663872 - Flags: review?(bzbarsky)
> OK, but in what situations would we have nonstatic members and no interface prototype
> object?

I believe for callback interfaces we'll get that.  But you could test by making it an assert instead of a check and seeing if it trips.  ;)
Comment on attachment 663872 [details] [diff] [review]
Part 0.6 v2

r=me.  hasLenientThis() can only be true for non-static stuff, so that part is fine, and you're right that the code makes more sense this way.
Attachment #663872 - Flags: review?(bzbarsky) → review+
Posted patch Part 1 v2 (obsolete) — Splinter Review
With requested changes
Attachment #663350 - Attachment is obsolete: true
Attachment #663880 - Flags: review?(bzbarsky)
Comment on attachment 663880 [details] [diff] [review]
Part 1 v2

This is the part 0.6 diff...
Attachment #663880 - Flags: review?(bzbarsky) → review-
Posted patch Part 1 v2Splinter Review
Attachment #663880 - Attachment is obsolete: true
Attachment #663887 - Flags: review?(bzbarsky)
Comment on attachment 663887 [details] [diff] [review]
Part 1 v2

>+++ b/dom/base/URL.cpp
>+using namespace mozilla;
>+using namespace mozilla::dom;

  namespace mozilla {
  namespace dom {

instead.

r=me with that.

Do we have a bug on implementing the autoRevoke thing, by the way?  Right now we just silently ignore it...  Not that we did anything different before.
Attachment #663887 - Flags: review?(bzbarsky) → review+
Attachment #663888 - Flags: review? → review?(jonas)
(In reply to Boris Zbarsky (:bz) from comment #26)
> Do we have a bug on implementing the autoRevoke thing, by the way?  Right
> now we just silently ignore it...  Not that we did anything different before.

I don't know.
Someone forgot to remove this when moving nsBlobProtocolHandler to content/base/public.
Attachment #663892 - Flags: review?
Attachment #663892 - Flags: review? → review?(jonas)
Jonas, I'm not sure if you're the right reviewer for this blob-URI stuff --- please reassign if you aren't.
Attachment #663894 - Attachment description: Part 5: Extend nsHostObjectProtocolHandler with support for 'mediastream' scheme → Part 4: Refactor nsBlobURI/nsBlobProtocolHandler to nsHostObjectURI/nsHostObjectProtocolHandler
Part 7 also extends <source> elements to support MediaStream URIs. I don't know why you'd want to do that, but it seems logical to support it.
Attachment #663897 - Flags: review?(cpearce) → review+
(In reply to Boris Zbarsky (:bz) from comment #26)
> Do we have a bug on implementing the autoRevoke thing, by the way?  Right
> now we just silently ignore it...  Not that we did anything different before.
Bug 773132, but it is stopping because it needs substantial spec changes.
I think we shouldn't touch the autoRevoke property of the aOptions parameter unless we actually support autoRevoke feature. Otherwise it will break feature detection using getter.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16953#c4
OK, I'll comment out autoRevoke, since that's easy and can't hurt.

However, I'm very skeptical that we'll be able to change createObjectURL to autoRevoke by default without breaking a lot of stuff, after all browser have interoperably *not* autoRevoked for a long time. Even if we find an acceptable way to spec autoRevoke, we'll need to make it false by default.
Should we support the options argument at all?  As in, is the fact that that argument will be a dictionary definitely decided?  If not, I think we should just drop the argument itself in the WebIDL.
IE10 has been already shipped with the optional dictionary argument.
Changing the dictionary to be empty broke Codegen.py. As far as I can tell from the WebIDL spec, empty dictionaries are allowed.
Attachment #664412 - Flags: review?(bzbarsky)
Comment on attachment 664412 [details] [diff] [review]
Part 0.6: Handle empty dictionary types in WebIDL bindings

This is part 0.7, right?  ;)

r=me
Attachment #664412 - Flags: review?(bzbarsky) → review+
Blocks: MSE

Comment 43

7 years ago
Has this work stalled out? We need this for the W3C Web Notifications patches and I'd like for W3C Web Notifications to land soon.
Josh, this is blocked on bug 763643, which is almost done, but is blocked on bug 778152, which is close to landing.
This is attachment 663872 [details] [diff] [review] rebased on top of attachment 676322 [details] [diff] [review] from bug 763643, I think I merged it correctly. Thanks for fixing that!
Comment on attachment 663894 [details] [diff] [review]
Part 4: Refactor nsBlobURI/nsBlobProtocolHandler to nsHostObjectURI/nsHostObjectProtocolHandler

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

So I take it that the goal here is that stream URLs won't use blob: as protocol?
Attachment #663894 - Flags: review?(jonas) → review+

Updated

7 years ago
Depends on: 813623
Duplicate of this bug: 820581
You need to log in before you can comment on or make changes to this bug.