855130 - Implement enough of the Media Source Extensions API to run the demo app

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Implement enough of the Media Source Extensions API so that the following works:

  var ms = new MediaSource();

  var video = document.querySelector('video');
  video.src = window.URL.createObjectURL(ms);

  ms.addEventListener('sourceopen', function(e) {
    ...
    var sourceBuffer = ms.addSourceBuffer('video/webm; codecs="vorbis,vp8"');
    sourceBuffer.append(oneVideoWebMChunk);
    ...
  }, false);

The bug URL links to a demo app that uses this to play a video.  It'll need slight modifications to work in Firefox, as it relies on the events including a "webkit" prefix.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip (applies to aed1da04448d)

This works with the slightly modified version of the demo hosted here: http://flim.org/~kinetik/mediasource_demo.html (changes: rename append to appendBuffer per current spec, removed webkit prefix from events).

Still to do: fix leaks and a shutdown hang.

Comment 2

[Yury Delendik (:yury)]

Windows build has problems with compiling content/media/mediasource/MediaSourceInputAdapter.h and .cpp: nsresult must be changed to NS_IMETHOD or NS_IMETHODIMP, e.g.

+  NS_IMETHOD Close();
...
+
+NS_IMETHODIMP
+MediaSourceInputAdapter::Close()
+{

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: wip v2 (applies to aed1da04448d)

Fix nsresult/NS_IMETHODIMP compile error on Win32 and switch InputAdapter from file to (unseekable) channel interface which allows multi-append playback to work with more decoders.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: fix some build issues found on try

I found some warnings-as-errors build issues with wip-v2: https://tbpl.mozilla.org/?tree=Try&rev=2a9e15d21a29.

This patch gives a clean try build: https://tbpl.mozilla.org/?tree=Try&rev=eb821c1c3b16.(I also noticed that mochitest-1 often hits that shutdown hang.)

Comment 5

[Geoff Brown [:gbrown]]

We are seeing some intermittent problems with the v2 patch when playing http://flim.org/~kinetik/mediasource_demo.html: On desktop, it usually almost completes, but not quite; on mobile devices, it sometimes stops quite soon, like at 1:03, and other times nearly completes.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: wip v3 (applies to 7b8ed29c6bc0)

Rebased, integrated gbrown's warning fixes, possibly fixed the stalling bug (bogus blocking calculation in Read()).

Comment 7

[Geoff Brown [:gbrown]]

The stalling bug I reported in Comment 5 seems to be resolved by wip-v3 -- it works great now!

Comment 8

[Shachar]

http://techcrunch.com/2013/04/15/netflix-html5-chromebooks/
http://www.webmonkey.com/2013/04/netflix-plans-to-ditch-silverlight-for-html5/
http://techblog.netflix.com/2013/04/html5-video-at-netflix.html

Comment 9

[Yury Delendik (:yury)]

On FF/Mac with mediasource+gstreamer enabled, the web page with MediaSource and MP4 file locks the browser, try http://async5.org/moz/msmp4.html .

Comment 10

[Yury Delendik (:yury)]

(FF/Mac/gstreamer) fragmented MP4 does not start playback until endOfStream is called, see example at http://async5.org/moz/story.html

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: minimal patch for leak debugging v0

I feel like I must be missing something obvious here, but I've got a leak that I can't work out and need some expertise to rescue me.  Attached is a minimized patch that implements MediaSource and a createObjectURL overload.  The included testcase, test_MediaSource.html, works correctly without the following line:

  ms.foo = null;

But with that line added, a whole lot of stuff leaks, including the MediaSource instance.  Am I missing some CC macro magic?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744478 [details] [diff] [review]
minimal patch for leak debugging v0

Hrm.  The CC macros look correct for a wrappercached object, so that part is OK.

I think you may be ending up with a cycle like so:

  MediaSource -> JSObject (preserved wrapper) -> Window's JSObject -> Document ->
    nsHostObjectProtocolHandler data entry -> MediaSource

At least I don't see anything obvious that would break this cycle... Kyle, do you know about this data entry stuff, or know who does?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> I think you may be ending up with a cycle like so:
> 
>   MediaSource -> JSObject (preserved wrapper) -> Window's JSObject ->
> Document ->
>     nsHostObjectProtocolHandler data entry -> MediaSource

Yes, I agree that that is what is likely happening.

> At least I don't see anything obvious that would break this cycle... Kyle,
> do you know about this data entry stuff, or know who does?

There is nothing that will break this cycle.  The existing consumer of this data entry stuff is the blob code.  Blobs don't preserve wrappers so this cycle is not possible.  The existing teardown code is located at http://hg.mozilla.org/mozilla-central/annotate/e474b6cfebce/content/base/src/nsDocument.cpp#l1501 in ~nsDocument, but obviously that won't help if you have a cycle.

So it sounds like we need to traverse through the protocol handler.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Matthew, are you OK giving that a shot?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I can find someone to do that if necessary.

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: traverse protocolhandler patch v0

Thanks for the explanations.  I'm not really sure what I'm doing here, but this seems to work.

Comment 17

[Matthew Gregan [:kinetik]]

14:36 < khuey|tw> kinetik: fwiw you can treat that f+ as an r+ if you like

Comment 18

[Matthew Gregan [:kinetik]]

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

Comment 19

[Matthew Gregan [:kinetik]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/502aca8c88e7

Crashing in tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=23244363&tree=Mozilla-Inbound

Comment 20

[Matthew Gregan [:kinetik]]

Simple fix: Traverse needs to null-check gDataTable.

Comment 21

[Matthew Gregan [:kinetik]]

Relanded with null-check (looks okay on try: https://tbpl.mozilla.org/?tree=Try&rev=c0224e2ec0b0):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8920210273

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c8920210273

Comment 23

[Matthew Gregan [:kinetik]]

Attachment: wip v4 (applies to 7e3a4ebcf067)

There's a lot of implementation work left to do, but this patch meets the goals set out in this bug and the rest of the work will hang off of bug 778617.  Posting for early feedback, I've got a couple more tests to add and some very minor cleanups to do in parallel, then I'd like to get this reviewed and landed.

Comment 24

[Matthew Gregan [:kinetik]]

(In reply to Yury Delendik (:yury) from comment #9)
> On FF/Mac with mediasource+gstreamer enabled, the web page with MediaSource
> and MP4 file locks the browser, try http://async5.org/moz/msmp4.html .

This should be fixed now.

(In reply to Yury Delendik (:yury) from comment #10)
> (FF/Mac/gstreamer) fragmented MP4 does not start playback until endOfStream
> is called, see example at http://async5.org/moz/story.html

Fragmented MP4 support will happen in a separate bug, it's quite a large chunk of work.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Does this have a spec?

I'd also appreciate a diff with 8 lines of context, if possible.

Comment 26

[Matthew Gregan [:kinetik]]

Attachment: wip v4.1 (applies to 22cb668fd727)

Sorry, updated with 8 lines of context.

The spec is hosted here: http://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html

Note that the current patch is based on the May 13th version; I'll update to June 1st before final review/landing.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 758422 [details] [diff] [review]
wip v4.1 (applies to 22cb668fd727)

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

Random comments from a first reading; I haven't compared with the spec for now.

::: content/base/public/nsHostObjectProtocolHandler.h
@@ +64,5 @@
>  
> +class nsMediaSourceProtocolHandler : public nsHostObjectProtocolHandler
> +{
> +public:
> +  NS_IMETHOD GetScheme(nsACString &result);

moz_override

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +15,5 @@
>  // -----------------------------------------------------------------------
>  // Hash table
>  struct DataInfo
>  {
> +  // mObject must be an nsIDOMBlob, nsIDOMMediaStream, or MediaSource

I'm not sure what "must be" means here

@@ +214,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsISupports> owner = do_QueryInterface(info->mPrincipal);
>  
> +  nsAutoString type;

Why nsAutoString? There's a good chance nsString could avoid the copy by refcounting the buffer, aiui

::: content/html/content/public/HTMLMediaElement.h
@@ +30,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/dom/TextTrack.h"
>  #include "mozilla/dom/TextTrackList.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/MediaPlaybackQuality.h"

Goes between mozilla/Attributes.h and mozilla/dom/TextTrack.h.

@@ -32,5 @@
>  #include "mozilla/dom/TextTrackList.h"
>  #include "mozilla/ErrorResult.h"
> -
> -// Define to output information on decoding and painting framerate
> -/* #define DEBUG_FRAME_RATE 1 */

Unrelated change?

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1108,5 @@
>      SetupSrcMediaStreamPlayback(static_cast<DOMMediaStream*>(stream.get()));
>      return NS_OK;
>    }
>  
> +  // XXX(kinetik): wire this up properly.

Bug number, if you don't fix it in this bug

@@ +1110,5 @@
>    }
>  
> +  // XXX(kinetik): wire this up properly.
> +  if (IsMediaSourceURI(mLoadingSrc)) {
> +    nsCOMPtr<MediaSource> source;

nsRefPtr

@@ +1120,5 @@
> +      const PRUnichar* params[] = { spec.get() };
> +      ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
> +      return rv;
> +    }
> +    mMediaSource = source.get();

Shouldn't need the .get(). (But you could use forget() to avoid and addref/release pair.)

@@ +1122,5 @@
> +      return rv;
> +    }
> +    mMediaSource = source.get();
> +    mMediaSource->AttachElement(this);
> +    // XXX(kinetik): bail here rather than relying on mediasource->channel conversion.

Ditto

@@ +3619,5 @@
> +  }
> +  nsPerformance* perf = window->GetPerformance();
> +  if (!perf) {
> +    return nullptr;
> +  }

NS_ENSURE_TRUE for both of those.

::: content/media/MediaPlaybackQuality.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "mozilla/dom/HTMLMediaElement.h"
> +#include "MediaPlaybackQuality.h"

Include this one first, and leave an empty line before and after.

@@ +15,5 @@
> +                                           DOMHighResTimeStamp aCreationTime,
> +                                           uint64_t aTotalFrames,
> +                                           uint64_t aDroppedFrames)
> +  : mElement(aElement), mCreationTime(aCreationTime),
> +    mTotalFrames(aTotalFrames), mDroppedFrames(aDroppedFrames)

One line each, and , lined up under :.

@@ +45,5 @@
> +  return mTotalFrames;
> +}
> +
> +uint64_t
> +MediaPlaybackQuality::DroppedVideoFrames()

These three can be inlined and made const

::: content/media/MediaPlaybackQuality.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Copy the boilerplate from <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line> for all new files. (Including 'file,' on the third line, tab-width: 8 in the emacs modeline and the vim modeline.)

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +#define MOZILLA_MEDIAPLAYBACKQUALITY_H_

mozilla_dom_MediaPlaybackQuality_h

@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +#define MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +
> +#include "mozilla/dom/MediaPlaybackQualityBinding.h"

Don't include this in the header, but in the .cpp

@@ +13,5 @@
> +namespace dom {
> +
> +class HTMLMediaElement;
> +
> +class MediaPlaybackQuality MOZ_FINAL : public nsISupports, public nsWrapperCache

", public nsWrapperCache" on its own line, with , lined up with :

::: content/media/mediasource/MediaSource.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "MediaSource.h"

Empty line before and after this

@@ +18,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +nsresult
> +MediaSource::GetType(nsAString& aType)

Why return nsresult?

@@ +25,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +MediaSource::GetInternalStream(nsIInputStream** aStream)

Ditto. This can return the stream directly.

@@ +33,5 @@
> +  adapter.forget(aStream);
> +  return NS_OK;
> +}
> +
> +/*static*/ already_AddRefed<MediaSource>

Nit: /* static */

@@ +50,5 @@
> +already_AddRefed<SourceBufferList>
> +MediaSource::SourceBuffers()
> +{
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);

MOZ_ASSERT_IF(mReadyState == dom::ReadyState::Closed, mSourceBuffers->Length() == 0);

(You could also add IsEmpty().)

@@ +53,5 @@
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);
> +  }
> +  nsRefPtr<SourceBufferList> list = mSourceBuffers;
> +  return list.forget();

No reason for the AddRef here. Just return a raw pointer.

@@ +63,5 @@
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);
> +  }
> +  nsRefPtr<SourceBufferList> list = mActiveSourceBuffers;
> +  return list.forget();

Ditto (×2)

@@ +96,5 @@
> +  }
> +  DurationChange(aDuration, aRv);
> +  if (aRv.Failed()) {
> +    return;
> +  }

That's hardly useful at the end of the function

@@ +107,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +    return nullptr;
> +  }
> +  // Check aType against HTMLMediaElement list of MIME types.
> +  // XXX: Further restrict this to formats in the spec.

Fix or file a bug (and note the number)

@@ +112,5 @@
> +  if (HTMLMediaElement::GetCanPlay(aType) == CANPLAY_NO) {
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return nullptr;
> +  }
> +  // XXX: temporary limit.

Ditto

@@ +138,5 @@
> +    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
> +    return;
> +  }
> +  // If aSourceBuffer is updating:
> +  if (aSourceBuffer.Updating()) {

You're inconsistent: you use both sourceBuffer and aSourceBuffer

@@ +162,5 @@
> +  }
> +  // Remove aSourceBuffer from sourceBuffers and fire "removesourcebuffer" at sourceBuffers
> +  mSourceBuffers->Remove(sourceBuffer);
> +  aSourceBuffer.Detach();
> +  // Free all resources associated with aSourceBuffer

I assume all these comments will be filled in later.

@@ +180,5 @@
> +/*static*/ bool
> +MediaSource::IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType)
> +{
> +  if (aType.IsEmpty()) {
> +    return false;

Interesting. Can't HTMLMediaElement::GetCanPlay deal with empty strings?

@@ +196,5 @@
> +  NotifyListeners();
> +}
> +
> +void
> +MediaSource::AttachElement(HTMLMediaElement* aElement)

(This isn't called yet?)

@@ +201,5 @@
> +{
> +  LOG(PR_LOG_DEBUG, ("%p Attaching element %p", this, aElement));
> +  MOZ_ASSERT(aElement);
> +  mElement = aElement;
> +  if (mReadyState != dom::ReadyState::Closed) {

Btw, you don't need the dom:: anywhere.

@@ +232,5 @@
> +}
> +
> +MediaSource::MediaSource(nsPIDOMWindow* aWindow)
> +  : mElement(nullptr),
> +    mDuration(UnspecifiedNaN()),

, to the front

@@ +237,5 @@
> +    mMonitor("mozilla::dom::MediaSource::mMonitor"),
> +    mReadyState(dom::ReadyState::Closed)
> +{
> +  mSourceBuffers = new SourceBufferList(this);
> +  mActiveSourceBuffers = new SourceBufferList(this);

Why not use initializers for those?

@@ +247,5 @@
> +#endif
> +
> +  MOZ_ASSERT(aWindow);
> +  nsDOMEventTargetHelper::BindToOwner(aWindow);
> +  SetIsDOMBinding();

You can use the nsDETH constructor that takes a window instead.

@@ +387,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(MediaSource)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> +
> +}
> +}

"} // namespace foo" (and anywhere else I missed it)

::: content/media/mediasource/MediaSource.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIASOURCE_H_
> +#define MOZILLA_MEDIASOURCE_H_

mozilla_dom_MediaSource_h

@@ +6,5 @@
> +#define MOZILLA_MEDIASOURCE_H_
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/Monitor.h"
> +#include "mozilla/dom/HTMLMediaElement.h"

Check if you need this include

@@ +23,5 @@
> +class SourceBufferList;
> +class SourceBuffer;
> +
> +template <typename T>
> +class AsyncEventRunnner : public nsRunnable

Does this need to be templated?

@@ +28,5 @@
> +{
> +public:
> +  AsyncEventRunnner(T* aTarget, const nsAString& aName)
> +    : mTarget(aTarget),
> +      mName(aName)

, to the front

@@ +31,5 @@
> +    : mTarget(aTarget),
> +      mName(aName)
> +  {}
> +
> +  NS_IMETHOD Run() {

{ on its own line

@@ +71,5 @@
> +  static bool IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType);
> +
> +  void AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv);
> +
> +  // Semi-private, for MSIA only.

MSIA?

@@ +74,5 @@
> +
> +  // Semi-private, for MSIA only.
> +  nsTArray<uint8_t> const& GetData() { return mData; }
> +  Monitor& GetMonitor() { return mMonitor; }
> +  bool AppendDone() { return mReadyState == dom::ReadyState::Closed; }

Please use the 4-line form

@@ +97,5 @@
> +  void DurationChange(double aNewDuration, ErrorResult& aRv);
> +  void EndOfStreamInternal(const Optional<EndOfStreamError>& aError, ErrorResult& aRv);
> +
> +
> +  HTMLMediaElement* mElement;

What's the ownership model here? Document.

@@ +112,5 @@
> +
> +  nsTArray<nsRefPtr<MediaSourceInputAdapter> > mAdapters;
> +
> +  // Protected by monitor.
> +  nsTArray<uint8_t> mData;

Consider the ordering of these members; biggest-to-smallest tends to waste the least.

::: content/media/mediasource/MediaSourceInputAdapter.cpp
@@ +27,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::Available(uint64_t* _retval)

aRetval or aAvailable

@@ +40,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::Read(char* aBuf, uint32_t aCount, uint32_t* _retval)

Ditto

@@ +47,5 @@
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
> +                                      uint32_t aCount, uint32_t* _retval)

Ditto

@@ +62,5 @@
> +  }
> +
> +  uint32_t count = std::min(aCount, available);
> +  nsresult rv = aWriter(this, aClosure,
> +                        reinterpret_cast<const char*>(mMediaSource->GetData().Elements() + mOffset),

Can this be &mMediaSource->GetData()[mOffset]?

@@ +116,5 @@
> +void
> +MediaSourceInputAdapter::NotifyListener()
> +{
> +  if (mCallback && (mMediaSource->GetData().Length() - mOffset >= mNotifyThreshold ||
> +                    mMediaSource->AppendDone() || mClosed)) {

Early return. And maybe a note what the second half means; it's not immediately obvious.

@@ +139,5 @@
> +}
> +
> +MediaSourceInputAdapter::MediaSourceInputAdapter(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource),
> +    mOffset(0),

Commas

@@ +154,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MediaSourceInputAdapter)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +  NS_INTERFACE_MAP_ENTRY(nsIInputStream)
> +  NS_INTERFACE_MAP_ENTRY(nsIAsyncInputStream)
> +  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(MediaSourceInputAdapter)

This should be included in NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION. (But double-check.)

::: content/media/mediasource/MediaSourceInputAdapter.h
@@ +31,5 @@
> +  uint32_t mNotifyThreshold;
> +
> +  nsRefPtr<MediaSource> mMediaSource;
> +  int64_t mOffset;
> +  bool mClosed;

These take 6 words on x64, but could fit in 5.

::: content/media/mediasource/SourceBuffer.cpp
@@ +44,5 @@
> +
> +double
> +SourceBuffer::TimestampOffset()
> +{
> +  return mTimestampOffset;

const and inline?

@@ +118,5 @@
> +
> +void
> +SourceBuffer::AppendBuffer(ArrayBuffer& aData, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(sizeof(*aData.Data()) == 1);

Given that you're passing this to a function that takes uint8_t*, I think you're fine.

@@ +184,5 @@
> +}
> +
> +SourceBuffer::SourceBuffer(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource),
> +    mAppendMode(AppendMode::Segments),

Commas

@@ +192,5 @@
> +    mTimestampOffset(0),
> +    mAttached(false)
> +{
> +  MOZ_ASSERT(aMediaSource);
> +  nsDOMEventTargetHelper::BindToOwner(aMediaSource->GetParentObject());

Constructor

::: content/media/mediasource/SourceBuffer.h
@@ +82,5 @@
> +  double mAppendWindowEnd;
> +
> +  double mTimestampOffset;
> +
> +  bool mAttached;

The usual

::: content/media/mediasource/SourceBufferList.cpp
@@ +20,5 @@
> +{
> +  if (aIndex < mSourceBuffers.Length()) {
> +    aFound = true;
> +    nsRefPtr<SourceBuffer> buf = mSourceBuffers[aIndex];
> +    return buf.forget();

I see no reason for the addref

@@ +33,5 @@
> +  return mSourceBuffers.Length();
> +}
> +
> +void
> +SourceBufferList::Append(nsRefPtr<SourceBuffer> aSourceBuffer)

nsRefPtr arguments? Why?

@@ +43,5 @@
> +void
> +SourceBufferList::Remove(nsRefPtr<SourceBuffer> aSourceBuffer)
> +{
> +  DebugOnly<bool> removed = mSourceBuffers.RemoveElement(aSourceBuffer);
> +  MOZ_ASSERT(removed);

Could also use MOZ_ALWAYS_TRUE

@@ +112,5 @@
> +SourceBufferList::SourceBufferList(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource)
> +{
> +  MOZ_ASSERT(aMediaSource);
> +  nsDOMEventTargetHelper::BindToOwner(aMediaSource->GetParentObject());

Constructor

::: content/media/mediasource/SourceBufferList.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_SOURCEBUFFERLIST_H_
> +#define MOZILLA_SOURCEBUFFERLIST_H_

As usual

@@ +7,5 @@
> +
> +#include "MediaSource.h"
> +#include "SourceBuffer.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/dom/SourceBufferListBinding.h"

Do you need this?

::: content/media/mediasource/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PARALLEL_DIRS += ['test']

I don't think we're entirely consistent yet, but

PARALLEL_DIRS += [
    'test'
]

::: content/media/mediasource/test/Makefile.in
@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES = \
> +		test_MediaSource.html \
> +		$(NULL)

Indent by two spaces

::: content/media/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PARALLEL_DIRS += ['mediasource', 'webaudio']

PARALLEL_DIRS += [
    'mediasource',
    'webaudio'
]

::: dom/bindings/Bindings.conf
@@ +598,5 @@
>  
> +'MediaSource': [{
> +},
> +{
> +    'nativeType': 'JSObject',

Gah.

::: dom/webidl/MediaSource.webidl
@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +enum ReadyState {

This exposes a dom::ReadyState... Please call it something more specific.

@@ +25,5 @@
> +interface MediaSource : EventTarget {
> +  readonly attribute SourceBufferList sourceBuffers;
> +  readonly attribute SourceBufferList activeSourceBuffers;
> +  readonly attribute ReadyState readyState;
> +  [SetterThrows] attribute unrestricted double duration;

All the Mozilla-specific extended attributes go on their own line.

::: layout/build/nsLayoutModule.cpp
@@ +272,5 @@
>  NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsDOMFileReader, Init)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsFormData)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsBlobProtocolHandler)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaStreamProtocolHandler)
> +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)

Do we need all this?

Comment 28

[Matthew Gregan [:kinetik]]

(In reply to :Ms2ger from comment #27)
> I assume all these comments will be filled in later.

In a later bug, yep.

> (This isn't called yet?)

Called from HTMLMediaElement.

> Btw, you don't need the dom:: anywhere.

It's required to disambiguate the ReadyState enum from ReadyState().  Once I rename the ReadyState enum as requested below it won't be necessary.

> Why not use initializers for those?

I did originally, it causes warnings.  See comment 4.

> Does this need to be templated?

Not without introducing a common interface for the users.  If I removed the logging from DispatchSimpleEvent I could remove the templating and call DispatchTrustedEvent (common on nsDETH) directly.

> Gah.

Required for the MediaSource overload of URL::createObjectURL. :-(

> This exposes a dom::ReadyState... Please call it something more specific.

MediaSourceReadyState?

Does the same apply for the other new enums (AppendMode, EndOfStreamError)?

> > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> 
> Do we need all this?

Not sure how to find out the answer to this.

Comment 29

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

(In reply to Matthew Gregan [:kinetik] from comment #28)
> > This exposes a dom::ReadyState... Please call it something more specific.
> 
> MediaSourceReadyState?

Yeah!

> Does the same apply for the other new enums (AppendMode, EndOfStreamError)?

I think so.

> > > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> > 
> > Do we need all this?
> 
> Not sure how to find out the answer to this.

Maybe we don't, but just put it in for now.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> (In reply to Matthew Gregan [:kinetik] from comment #28)
> > > This exposes a dom::ReadyState... Please call it something more specific.
> > 
> > MediaSourceReadyState?
> 
> Yeah!

Sounds good to me too.

> > Does the same apply for the other new enums (AppendMode, EndOfStreamError)?
> 
> I think so.

I feel less strongly about those; these names aren't overloaded so heavily.

> > > > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> > > 
> > > Do we need all this?
> > 
> > Not sure how to find out the answer to this.
> 
> Maybe we don't, but just put it in for now.

wfm

Comment 31

[Matthew Gregan [:kinetik]]

Attachment: patch v4.2 (applies to 22cb668fd727)

Thanks for the feedback!  This patch addresses everything that I didn't cover in comment 28.

Comment 32

[Matthew Gregan [:kinetik]]

Attachment: patch v4.2 (applies to cc35f8929768)

Same, but rebased to current tip (minor changes to content/media/moz.build).

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(I can take another look, probably some time next week, but I should mention that I'm probably not going to feel comfortable granting r+.)

Comment 34

[Matthew Gregan [:kinetik]]

Should I convert the request to a feedback? and maybe add Boris for review?

Comment 35

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

If you break out the host-object stuff into its own patch, khuey can review that.

I think I can review the rest. I did a lot of Web Audio reviews which is similar in scope to this code.

Comment 36

[Matthew Gregan [:kinetik]]

Attachment: patch v4.4 (applies to 1790f40f71f0)

Split HostObjectProtocolHandler and URL changes out.  Improved mochitest to cover all of the initial functionality this bug targets.

Comment 37

[Matthew Gregan [:kinetik]]

Attachment: hostobjectprotocolhandler patch v1.0 (applies to 1790f40f71f0)

Applies on top of main patch.  Add MediaSource to HostObjectProtocolHandler and extend URL::createObjectURL to support MediaSource.

Comment 38

[Matthew Gregan [:kinetik]]

Comment on attachment 763990 [details] [diff] [review]
patch v4.4 (applies to 1790f40f71f0)

Obsoleting these patches and splitting into smaller chunks.

Comment 39

[Matthew Gregan [:kinetik]]

Attachment: MSE p1 VideoPlaybackQuality v1.0

Comment 40

[Matthew Gregan [:kinetik]]

Attachment: MSE p2 MediaSource v1.0

Comment 41

[Matthew Gregan [:kinetik]]

Attachment: MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0

Comment 42

[Matthew Gregan [:kinetik]]

Attachment: MSE p4 MediaElement integration v1.0

Comment 43

[Matthew Gregan [:kinetik]]

Attachment: MSE p5 MSE tests v1.0

Comment 44

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

Comment on attachment 764582 [details] [diff] [review]
MSE p1 VideoPlaybackQuality v1.0

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

::: content/html/content/src/HTMLVideoElement.cpp
@@ +264,5 @@
> +    corruptedFrames = totalFrames - stats.GetDecodedFrames();
> +  }
> +  VideoFrameContainer* container = GetVideoFrameContainer();
> +  if (container) {
> +    playbackJitter = container->GetFrameDelay();

The spec says that this should be the sum of the absolute values of the delay for each frame. This is only the delay for the last frame (and no absolute values in sight).

::: content/media/VideoPlaybackQuality.h
@@ +15,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class VideoPlaybackQuality MOZ_FINAL : public nsISupports
> +                                     , public nsWrapperCache

I think this doesn't need to inherit from nsISupports. See http://lxr.mozilla.org/mozilla-central/source/dom/encoding/TextDecoder.h for an example of how to do this without nsISupports. But let me check with someone to find out when we are/aren't supposed to use nsISupports with WebIDL.

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 764582 [details] [diff] [review]
MSE p1 VideoPlaybackQuality v1.0

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

::: content/html/content/src/HTMLVideoElement.cpp
@@ +247,5 @@
> +already_AddRefed<dom::VideoPlaybackQuality>
> +HTMLVideoElement::VideoPlaybackQuality()
> +{
> +  nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> +  NS_ENSURE_TRUE(window, nullptr);

This attribute isn't nullable in the IDL, so this will crash. (Probably not exploitable anymore, but who knows...)

@@ +249,5 @@
> +{
> +  nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> +  NS_ENSURE_TRUE(window, nullptr);
> +  nsPerformance* perf = window->GetPerformance();
> +  NS_ENSURE_TRUE(perf, nullptr);

Another crash.

::: content/media/VideoPlaybackQuality.h
@@ +15,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class VideoPlaybackQuality MOZ_FINAL : public nsISupports
> +                                     , public nsWrapperCache

I don't see a reason why this should inherit from nsISupports, on first sight. And if JS can only ever get hold of newly created objects, it can probably be an owned class. (Might even be able to drop the refcounting.)

::: dom/webidl/HTMLVideoElement.webidl
@@ +45,5 @@
>    // True if the video has an audio track available.
>    readonly attribute boolean mozHasAudio;
>  };
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-VideoPlaybackQuality

https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-HTMLVideoElement

@@ +48,5 @@
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-VideoPlaybackQuality
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled"]
> +  readonly attribute VideoPlaybackQuality videoPlaybackQuality;

This always returns a new object? Add [Creator], and complain about the spec, because something that returns a new object every time should be a method, not an attribute.

@@ +50,5 @@
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled"]
> +  readonly attribute VideoPlaybackQuality videoPlaybackQuality;
> +};
> + 

Trailing whitespace

Comment 46

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

Comment on attachment 764583 [details] [diff] [review]
MSE p2 MediaSource v1.0

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

Mostly looks fine.

One big spec issue I realized here: I think appendBuffer() should neuter the array parameter and take ownership of the underlying data buffer. This would reduce copies and memory usage, potentially by a lot. Does that make sense? If so, please bring it up on the list ASAP.

::: content/media/mediasource/MediaSource.cpp
@@ +24,5 @@
> +
> +nsString
> +MediaSource::GetType()
> +{
> +  return mContentType;

Might as well be inline in the header. Also I think we can return const nsString& here?

@@ +28,5 @@
> +  return mContentType;
> +}
> +
> +already_AddRefed<nsIInputStream>
> +MediaSource::GetInternalStream()

Better to call this CreateInternalStream?

@@ +99,5 @@
> +  if (!IsTypeSupportedInternal(aType, aRv)) {
> +    return nullptr;
> +  }
> +  // XXX(kinetik): Bug 881512. Temporary limit until multiple decoders are supported.
> +  if (mSourceBuffers->Length() == 1) {

Make this >= 1 ?

@@ +183,5 @@
> +MediaSource::AttachElement(HTMLMediaElement* aElement)
> +{
> +  LOG(PR_LOG_DEBUG, ("%p Attaching element %p", this, aElement));
> +  MOZ_ASSERT(aElement);
> +  mElement = aElement;

Does the spec explicitly say anywhere that there can be only one one element attached at a time? I don't see that it does. That should be fixed in the spec, and it will have to say what happens when someone tries to attach the same MediaSource URI to multiple media elements. I think at least here we should have AttachElement return false and fail if it can't attach, and have the media element try to recover from that.

We have to be careful that since mElement is weak, we don't allow applications to detect the GC of the HTMLMediaElement while the MediaSource remains alive. It might be better to make mElement an nsRefPtr and use CC here.

@@ +326,5 @@
> +    // DurationChange(highestDurationOfSourceBuffers, aRv);
> +    // if (aRv.Failed()) {
> +    //   return;
> +    // }
> +    // Notify media element that all data is now available.

Sometimes it's not 100% clear that a comment is a TODO. I suggest explicitly adding TODO everywhere.

::: content/media/mediasource/MediaSource.h
@@ +29,5 @@
> +template <typename T>
> +class AsyncEventRunnner : public nsRunnable
> +{
> +public:
> +  AsyncEventRunnner(T* aTarget, const nsAString& aName)

This really belongs in some other file, but I guess it can stay here for now.

@@ +42,5 @@
> +  }
> +
> +private:
> +  nsRefPtr<T> mTarget;
> +  nsString mName;

How about just const char*, since these are always ASCII string literals? Pass const char* to DispatchSimpleEvent too.

@@ +73,5 @@
> +
> +  void EndOfStream(const Optional<MediaSourceEndOfStreamError>& aError, ErrorResult& aRv);
> +  static bool IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType);
> +
> +  void AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv);

Use comments to delimit the WebIDL methods from the rest of the class.

@@ +94,5 @@
> +
> +  void AttachElement(HTMLMediaElement* aElement);
> +  void DetachElement();
> +
> +  explicit MediaSource(nsPIDOMWindow* aWindow);

Make this protected/private?

@@ +120,5 @@
> +
> +  // Protected by monitor.
> +  nsTArray<uint8_t> mData;
> +
> +  Monitor mMonitor;

Add comments explaining what mMonitor protects.

Comment 47

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

Comment on attachment 764585 [details] [diff] [review]
MSE p4 MediaElement integration v1.0

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

I think it would make sense to support mozSrcObject = mediaSource, but that can be done separately.

Comment 48

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

Comment on attachment 764586 [details] [diff] [review]
MSE p5 MSE tests v1.0

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

Very nice!

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Add [Creator], and complain about the spec

You can't add [Creator] on an attribute, because that's nonsense.

Returning a new object from this thing each time is just not ok.  We need to either return a live object or have this be a method.

Comment 50

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

I think this should probably be a live object.

Comment 51

[Matthew Gregan [:kinetik]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> I think it would make sense to support mozSrcObject = mediaSource, but that
> can be done separately.

Bug 886194.

Comment 52

[Matthew Gregan [:kinetik]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> One big spec issue I realized here: I think appendBuffer() should neuter the
> array parameter and take ownership of the underlying data buffer. This would
> reduce copies and memory usage, potentially by a lot. Does that make sense?
> If so, please bring it up on the list ASAP.

w3.org Bug 22432.

> Does the spec explicitly say anywhere that there can be only one one element
> attached at a time? I don't see that it does. That should be fixed in the
> spec, and it will have to say what happens when someone tries to attach the
> same MediaSource URI to multiple media elements. I think at least here we
> should have AttachElement return false and fail if it can't attach, and have
> the media element try to recover from that.

w3.org Bug 22430.

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #49)
> Returning a new object from this thing each time is just not ok.  We need to
> either return a live object or have this be a method.

w3.org Bug 22431.

Comment 53

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 764584 [details] [diff] [review]
MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0

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

I'm surprised this compiles.  There's a worker thread implementation of URL, how did you get away without modifying that?

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #53)
> Comment on attachment 764584 [details] [diff] [review]
> MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0
> 
> Review of attachment 764584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm surprised this compiles.  There's a worker thread implementation of URL,
> how did you get away without modifying that?

It hits the JSObject& overload, I suppose.

Comment 55

[Matthew Gregan [:kinetik]]

Bindings.conf:

'MediaSource': [{
    'resultNotAddRefed': [ 'sourceBuffers', 'activeSourceBuffers' ],
},
{
    'nativeType': 'JSObject',
    'workers': True,
    'skipGen': True
}],

...so, yeah.  I copied DOMMediaStream, so presumably that's broken for the worker thread implementation of URL too?

Comment 56

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

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #49)
> > Add [Creator], and complain about the spec
> 
> You can't add [Creator] on an attribute, because that's nonsense.
> 
> Returning a new object from this thing each time is just not ok.  We need to
> either return a live object or have this be a method.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=22431 points out that HTMLMediaElement.buffered/played/seekable are already using [Creator]...

Comment 57

[Boris Zbarsky [:bzbarsky]]

Huh.  I thought we disallowed that!  We should.

Comment 58

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

We'd need a spec change, since they are currently specced to return fresh objects. See
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-buffered
Can you point this out on WHATWG and explain why this is bogus? Because I don't fully understand the problem.

Comment 59

[Boris Zbarsky [:bzbarsky]]

We don't need a spec change to disallow [Creator] on attributes...  I did file https://www.w3.org/Bugs/Public/show_bug.cgi?id=22471 earlier today, though.

Comment 60

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

Oh, we can remove [Creator] but still return a new object every time? Great, let's do that here then :-).

Comment 61

[Boris Zbarsky [:bzbarsky]]

[Creator] just tells binding code that you promise to return a new object every time, so we can enforce that you're not returning non-wrappercached objects unless you make that promise.  You can, of course, not promise but still do.  ;)

But my real concern here is that this API seems like a bad API from a JS developer perspective, and we should push back on the spec for it...

Comment 62

[Matthew Gregan [:kinetik]]

Attachment: MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.1

Comment 63

[Matthew Gregan [:kinetik]]

Attachment: MSE p2 - Implement a minimal working subset of the MediaSource Extensions API. v1.1

Comment 64

[Matthew Gregan [:kinetik]]

Attachment: MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.1

Comment 65

[Matthew Gregan [:kinetik]]

Attachment: MSE p4 Integrate MediaSource into HTMLMediaElement. v1.1

Comment 66

[Matthew Gregan [:kinetik]]

Attachment: MSE p5 Add initial tests for MediaSource Extensions implementation. v1.1

Comment 67

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

Comment on attachment 768142 [details] [diff] [review]
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.1

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

::: dom/webidl/HTMLVideoElement.webidl
@@ +47,5 @@
>  };
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-HTMLVideoElement
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled", Creator]

Remove Creator attribute.

Comment 68

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/184ef886365d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6a6ac3a9d5

Comment 69

[Matthew Gregan [:kinetik]]

Drop [Creator], did this in my local changes but forgot and landed the patches from this bug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/59857a435c3b

Comment 70

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 768145 [details] [diff] [review]
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.1

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

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +314,5 @@
> +  NS_ASSERTION(IsMediaSourceURI(aURI), "Only call this with mediasource URIs");
> +
> +  *aSource = nullptr;
> +
> +  nsCOMPtr<mozilla::dom::MediaSource> source = do_QueryInterface(GetDataObject(aURI));

This doesn't actually do what you think it does.  mozilla::dom::MediaSource is not an interface, so do_QueryInterface can't QI to it.  MediaSource singly inherits from nsIDOMEventTarget (via nsDOMEventTargetHelper and friends) so the do_QueryInterface is actually going to QI to nsIDOMEventTarget and then static_cast to a MediaSource.  This works today but if someone ever adds another type for GetDataObject to return that inherits from nsIDOMEventTargetHelper this will do very bad things.

You should use the declare/define iid macros on MediaSource to make it actually an interface.

Comment 71

[Matthew Gregan [:kinetik]]

Backed out due to burninating:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acec254f82b1

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

Comment 72

[Matthew Gregan [:kinetik]]

Attachment: MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.2

Fix build error by removing unused GetFrameStatistics from HTMLMediaElement.

Comment 73

[Matthew Gregan [:kinetik]]

Attachment: MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.2

Add IID goop to make MediaSource a real interface.

Comment 74

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=682bfc7c7c3c

Comment 75

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a300d1d1b23
https://hg.mozilla.org/integration/mozilla-inbound/rev/1788eb4682b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d59f1d099d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba3d1f2c086
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa1f7715c19

Comment 76

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1df349338b08 for Windows build failures that may well just have been a need to clobber, dunno.

Comment 77

[Matthew Gregan [:kinetik]]

Try push of the same changesets that were just backed out:

https://tbpl.mozilla.org/?tree=Try&rev=403c3ca5ee70

Comment 78

[Phil Ringnalda (:philor)]

Also Android mochitest-2 bustage.

Comment 79

[Matthew Gregan [:kinetik]]

That try push is fairly green, so It looks like it just needs a clobber.

The Android M2 is real though, and not caught by the try test, so thanks for pointing that out:  https://tbpl.mozilla.org/php/getParsedLog.php?id=24818421&tree=Mozilla-Inbound

Comment 80

[Matthew Gregan [:kinetik]]

Pushed with a clobber:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c44734032b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/26bac32e8738
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7b6ea49038
https://hg.mozilla.org/integration/mozilla-inbound/rev/03eb7be93bb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af441153a54
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3777ac820c

The Android M2 failure was addressed by disabling the test on that platform and filing bug 889712 to fix it.

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c44734032b0
https://hg.mozilla.org/mozilla-central/rev/26bac32e8738
https://hg.mozilla.org/mozilla-central/rev/3a7b6ea49038
https://hg.mozilla.org/mozilla-central/rev/03eb7be93bb0
https://hg.mozilla.org/mozilla-central/rev/5af441153a54
https://hg.mozilla.org/mozilla-central/rev/8b3777ac820c