1150305 - sourcebuffer.buffered always return a new TimeRange object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Jean-Yves Avenard [:jya]]

The spec were a tad unclear and gave room to interpretation.

However, this has changed following bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=27790

sourcebuffer.buffered must return the same object ; not a new TimeRange each time it is called.

Now the summary of the change is:
http://w3c.github.io/media-source/index.html#widl-SourceBuffer-buffered
"Bug 27790 - Make .buffered attributes return the same object if nothing has changed."

However, the description of the buffered attribute:
http://w3c.github.io/media-source/index.html#widl-SourceBuffer-buffered
"5.        If intersection ranges does not contain the exact same range information as the current value of this attribute, then update the current value of this attribute to intersection ranges.
6.        Return the current value of this attribute.
"

the intent is to return the exact same object if it hasn't changed, not a new TimeRange each time. And so should we

Comment 1

[Guang-De Lin]

Attachment: 0001-sourcebuffer.buffered-returns-the-same-TimeRanges-ob.patch

Hello, I'm a newbie here and picked this bug as my first practice.
The attached file is my git patch and needs for your feedback.

Main idea is to make TimeRanges to inherit from nsWrapperCache, participate in cycle collection, and determine if buffered TimeRanges changes in SourceBuffer::GetBuffered().

I'm not sure if it's a good practice that TimeRanges::GetParentObject() returns a nsISupports pointer. For now two class cooperate with TimeRanges: SourceBuffer & HTMLMediaElement. In some const functions of HTMLMediaElement, I must const_cast to pass nsISupports* into TimeRanges constructor.

Thanks for your response in advance.

Comment 2

[Guang-De Lin]

Attachment: mse.js

I modified this MSE demo(http://html5-demos.appspot.com/static/media-source.html) and the attached JS file is what I verify with if my patch works.

It simply keep a buffered object reference and compare with current value of sourcebuffer.buffered, when 1. file chunks has been added into source buffer, 2. nothing happens.

On my Nightly everything worked fine, tested with WebM and fragmented MP4. YouTube MSE test passed.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8668342 [details] [diff] [review]
0001-sourcebuffer.buffered-returns-the-same-TimeRanges-ob.patch

Roc, I don't know enough about nsWrapperCache / cycle collection to provide very relevant comments.
The extent of my feedback would be on the form rather than the content.

Comment 4

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

Comment on attachment 8668342 [details] [diff] [review]
0001-sourcebuffer.buffered-returns-the-same-TimeRanges-ob.patch

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

Looks good to me, but bz should take a look for the DOM changes (and because he cares about this change...)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8668342 [details] [diff] [review]
0001-sourcebuffer.buffered-returns-the-same-TimeRanges-ob.patch

>+  nsRefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(const_cast<HTMLMediaElement*>(this)));

Any reason to not pass in OwnerDoc() as the owner, since we're just storing it as nsISupports anyway, and avoid the cast?

>+  , mBuffered(new TimeRanges(ToSupports(this)))

This is deeply suspect, since it refcounts the object before construction is complete.

It would probably be better to init to null here and to tolerate a null mBuffered in the getter (by doing the check for whether intersection matches currentValue only if mBuffered is not null).

>+  [Throws]
>   readonly attribute TimeRanges buffered;

This is probably fine.  If we cared, we could add one of the Pure/DependsOn/Affects annotations here, but I doubt getting this is ever hot enough to benefit from LICM and CSE, right?

r=me with at least the addref-in-constructor issue fixed.

Comment 6

[Guang-De Lin]

Attachment: sourcebuffer.buffered returns the same object if not changed

Comments addressed:

1. No special reason to take HTMLMediaElement as the owner, as it's my first thought since HTMLMediaElement creates TimeRanges objects.
Now it passes OwnerDoc() instead to avoid the const_cast.

2. mBufferd now is set nullptr in constructor and it'll determine in getter if it's null.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8670166 [details] [diff] [review]
sourcebuffer.buffered returns the same object if not changed

r=me

Comment 8

[Guang-De Lin]

Thanks for who helped me.

I'm preparing to be a mozilla committer and could jya help to assign this bug to me, please?
Thanks.

Comment 9

[Guang-De Lin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c84ecfc68ff

Comment 10

[Guang-De Lin]

Attachment: sourcebuffer.buffered returns the same object if not changed

Main changes:
Do traverse/unlink the new nsRefPtr<TimeRanges> in SourceBuffer.

I did mochitest dom/media/mediasource on my OSX 10.10 debug and no leakage there.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8674666 [details] [diff] [review]
sourcebuffer.buffered returns the same object if not changed

r=me

Comment 12

[Guang-De Lin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=710096fbe999

Comment 13

[Guang-De Lin]

Attachment: sourcebuffer.buffered returns the same object if not changed

Update the patch with commit messages.
The patch is actually the same with the last one reviewed by bz.

Comment 14

[Guang-De Lin]

(In reply to Guang-De Lin from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=710096fbe999

My try run here failed on mochitest dt2 due to timeout.
But recent try runs were easy to get timeout result on dt2.
So I fired a simple dt2 try run without my patch and it got timeout again.
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=16684cebea1e)
I think dt2 timeout was not related to my patch.

Set checkin-needed.

Comment 15

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

 out of 5 hunks FAILED -- saving rejects to file dom/html/HTMLMediaElement.cpp.rej
patching file dom/media/gtest/TestIntervalSet.cpp
Hunk #1 FAILED at 529
Hunk #2 FAILED at 604
Hunk #3 FAILED at 626
3 out of 3 hunks FAILED -- saving rejects to file dom/media/gtest/TestIntervalSet.cpp.rej
patching file dom/media/mediasource/SourceBuffer.cpp
Hunk #1 FAILED at 112
1 out of 3 hunks FAILED -- saving rejects to file dom/media/mediasource/SourceBuffer.cpp.rej

could you take a look, thanks!

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8675079 [details] [diff] [review]
sourcebuffer.buffered returns the same object if not changed

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

Make sure your patches are based on inbound too.

::: dom/html/TimeRanges.cpp
@@ +23,3 @@
>  
> +TimeRanges::TimeRanges(nsISupports* aParent)
> +  : mParent(aParent)

Would be much nicer to have a default constructor taking no argument that sets mParent to NULL so you wouldn't have to modify all the files not caring about it (like all the TimeIntervals gtest)

Comment 17

[Guang-De Lin]

(In reply to Jean-Yves Avenard [:jya] from comment #16)
> Comment on attachment 8675079 [details] [diff] [review]
> sourcebuffer.buffered returns the same object if not changed
> 
> Review of attachment 8675079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Make sure your patches are based on inbound too.

My try run worked 3 days ago... Anyway I'll fix it now.

> 
> ::: dom/html/TimeRanges.cpp
> @@ +23,3 @@
> >  
> > +TimeRanges::TimeRanges(nsISupports* aParent)
> > +  : mParent(aParent)
> 
> Would be much nicer to have a default constructor taking no argument that
> sets mParent to NULL so you wouldn't have to modify all the files not caring
> about it (like all the TimeIntervals gtest)

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class
It's said that "Returning null from GetParentObject is allowed in situations in which it's OK to associate the resulting object with a random global object for security purposes;  this is not usually ok for things that are exposed to web content.".

TimeRanges was used as an utility class and now participates in nsWrapperCache and cycle collection,
so what I think is not to have a default ctor taking no argument nor argument with nullptr as default value,
to make sure we exactly know who is TimeRanges' parent and pass it to ctor.

How do you think about this? Or it's fine to have ctor one argument with nullptr as default value?

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Guang-De Lin from comment #17)
> (In reply to Jean-Yves Avenard [:jya] from comment #16)
> > Comment on attachment 8675079 [details] [diff] [review]
> > sourcebuffer.buffered returns the same object if not changed
> > 
> > Review of attachment 8675079 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Make sure your patches are based on inbound too.
> 
> My try run worked 3 days ago... Anyway I'll fix it now.
> 
> > 
> > ::: dom/html/TimeRanges.cpp
> > @@ +23,3 @@
> > >  
> > > +TimeRanges::TimeRanges(nsISupports* aParent)
> > > +  : mParent(aParent)
> > 
> > Would be much nicer to have a default constructor taking no argument that
> > sets mParent to NULL so you wouldn't have to modify all the files not caring
> > about it (like all the TimeIntervals gtest)
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Adding_WebIDL_bindings_to_a_class
> It's said that "Returning null from GetParentObject is allowed in situations
> in which it's OK to associate the resulting object with a random global
> object for security purposes;  this is not usually ok for things that are
> exposed to web content.".
> 
> TimeRanges was used as an utility class and now participates in
> nsWrapperCache and cycle collection,
> so what I think is not to have a default ctor taking no argument nor
> argument with nullptr as default value,
> to make sure we exactly know who is TimeRanges' parent and pass it to ctor.
> 
> How do you think about this? Or it's fine to have ctor one argument with
> nullptr as default value?

That's not really my point and has nothing to do with WebIDL.

having to change every instance where you allocate a TimeRanges to TimeRanges(nullptr) seems unnecessary; it makes your patch much bigger than it needs to be and make you modify files that don't need to be: TestIntervalSet shouldn't be touched. The whole point of having gtest is to ensure no regression, by modifying it you break that assumption.

Having an additional ctor taking no arguments is IMHO the way to go.

Your default one-argument constructor will also fail the static analysers as you haven't marked it as explicit.

You have limited your try run to linux64 where the static analysers do not run. Expend it to all build platforms.

If you apply your patch as-is, you will get compilation failures on those platforms and it will get backed out; I guarantee it :)

Comment 19

[Guang-De Lin]

Attachment: sourcebuffer.buffered returns the same object if not changed

Merge Issue fixed:
s/nsRefPtr/RefPtr/

Main changes about the new patch:
Have an additional ctor without argument and leave gtest unmodified.

mochitest dom/media/mediasource: passed on my OSX 10.10.

Thanks for your feedback and I'll have my try run on all platform in the future.

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8675518 [details] [diff] [review]
sourcebuffer.buffered returns the same object if not changed

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

::: dom/html/TimeRanges.cpp
@@ +26,1 @@
>  {

Why can't we keep MOZ_COUNT_CTOR/DTOR?

::: dom/media/mediasource/SourceBuffer.cpp
@@ +311,5 @@
>    , mActive(false)
>    , mUpdateID(0)
>    , mReportedOffset(0)
>    , mType(aType)
> +  , mBuffered(nullptr)

you don't need to initialise mBuffered, it's a RefPtr the default constructor will do so for you.

Comment 21

[Guang-De Lin]

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> Comment on attachment 8675518 [details] [diff] [review]
> sourcebuffer.buffered returns the same object if not changed
> 
> Review of attachment 8675518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/TimeRanges.cpp
> @@ +26,1 @@
> >  {
> 
> Why can't we keep MOZ_COUNT_CTOR/DTOR?
> 

https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#152
says:
// Note that the following constructor/destructor logging macros are redundant
// for refcounted objects that log via the NS_LOG_ADDREF/NS_LOG_RELEASE macros.
// Refcount logging is preferred.

And now we use NS_IMPL_CYCLE_COLLECTING_ADDREF/RELEASE that include NS_LOG_ADDREF/RELEASE.
https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#660

What I knew was that MOZ_COUNT_CTOR/DTOR pair was redundant so I removed it.

> ::: dom/media/mediasource/SourceBuffer.cpp
> @@ +311,5 @@
> >    , mActive(false)
> >    , mUpdateID(0)
> >    , mReportedOffset(0)
> >    , mType(aType)
> > +  , mBuffered(nullptr)
> 
> you don't need to initialise mBuffered, it's a RefPtr the default
> constructor will do so for you.

Thanks for the notice and I'll have a small fix on it.

Comment 22

[Guang-De Lin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4c4e37a8eb2

Comment 23

[Guang-De Lin]

Attachment: sourcebuffer.buffered returns the same object if not changed.

Changes about the new patch:
Not to pass nullptr to RefPtr<TimeRanges> ctor since RefPtr default ctor does the same.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23dcd1ce4290
Here's the try submission and all test passed with some retries.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db71bd216371

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db71bd216371