1148102 - Convert WebMReader to MediaDataDemuxer and MediaDataDecoder's API

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

With the changes required to complete bug 1119208, the webm decoder needs to be converted into a the newer PlatformDecoderModule architecture.

We need a MP4Demuxer (to be renamed) equivalent that will feed samples to the MP4Reader (also to be renamed)

Comment 1

[Jean-Yves Avenard [:jya]]

The architecture is documented in bug 1148292 and the API for MediaDataDemuxer is documented in bug 1154164.

Comment 2

[Jan Gerber]

Attachment: WIP:_Convert_WebMReader_to_MediaDataDemuxer_and_MediaDataDecoder.patch

WIP: seeking still not fully working

This patch is on top of the patch for Bug 1104475 but splits the
audio demuxers into separate files.

The patch adds MediaDataDecoders for Vorbis, Opus and VP8/VP9
and a WebM MediaDataDemuxer.

Comment 3

[Jan Gerber]

Attachment: Bug_1148102_Convert_WebMReader_to_MediaDataDemuxer_and_MediaDataDecoder.patch

Comment 4

[Jan Gerber]

Attachment: Bug_1148102_Convert_WebMReader_to_MediaDataDemuxer_and_MediaDataDecoder.patch

- base on top of 1104475
- still deadlocks on remote seeks

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624161 [details] [diff] [review]
Bug_1148102_Convert_WebMReader_to_MediaDataDemuxer_and_MediaDataDecoder.patch

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

The decoders (VorbisDecoder/OpusDecoder etc) should be in platform/agnostic

remove reference to a base WebMDecoder class. it's confusing as they don't have to be used within webm (and won't always be).

Please split things in 3 parts:
- The WebM demuxer
- The new MediaDataDecoder
- The integration in the new WebMDecoder and the hook up with MediaFormatReader.

Don't remove WebMReader yet. You can open a new bug and do that there.


I'll only look at the WebM demuxer part at this stage.

::: dom/media/DecoderTraits.cpp
@@ +692,4 @@
>  #endif
>  #ifdef MOZ_WEBM
>    if (IsWebMType(aType)) {
> +    decoderReader = static_cast<MediaDecoderReader*>(new MediaFormatReader(aDecoder, new WebMDemuxer(aDecoder->GetResource())));

have a preference for that for the time being so we can use the old WebM reader until we're happy with the performance of the new one

::: dom/media/MediaFormatReader.cpp
@@ +445,5 @@
> +                                    mAudio.mCallback);
> +    } else {
> +      NS_WARNING("Unsupported Audio MimeType");
> +      return false;
> +    }

MediaFormatReader is to be content agnostic. It should never do anything related to a particular codec or mime type.

This should be all handled by the PlatformDecoderModule this is where you want to create the appropriate decoder.

@@ +472,5 @@
> +                                   mLayersBackendType,
> +                                   mDecoder->GetImageContainer());
> +      }
> +    } else if ((mInfo.mVideo.mMimeType.EqualsLiteral("video/webm; codecs=vp8") ||
> +             mInfo.mVideo.mMimeType.EqualsLiteral("video/webm; codecs=vp9"))) {

same as above.

You also don't want to check the mimetype literally. as there are plenty of people using mimetype such as video/webm; codecs=vp8,codecs=vp9 or something to that extent.

In any case, it shouldn't be done here.

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +700,4 @@
>  
>  #ifdef MOZ_WEBM
>    if (DecoderTraits::IsWebMType(aType)) {
> +    return static_cast<MediaDecoderReader*>(new MediaFormatReader(aDecoder, new WebMDemuxer(aDecoder->GetResource()), aBorrowedTaskQueue));

do a similar pref to media.format-reader.mp4 and media.mediasource.format-reader.mp4

so one media.format-reader.webm and media.mediasource.format-reader.webm

::: dom/media/webm/WebMBufferedParser.cpp
@@ +351,4 @@
>    }
>  }
>  
> +void WebMBufferedState::UpdateIndex(nsTArray<MediaByteRange>& aRanges, nsRefPtr<MediaResource> aResource)

MediaResource* seeing that the scope of aResource doesn't extend past UpdateIndex

@@ +387,5 @@
> +      }
> +    }
> +
> +    char* buffer = (char *)moz_xmalloc(length);
> +    nsresult rv = aResource->ReadFromCache(buffer, offset, length);

You can't use ReadFromCache / Read with a MediaResource ; they aren't thread safe.
While MediaResource itself is threadsafe if you get more than two objects accessing the MediaResource at the same time, you have no way to know where one thread will read from and where the other will.

This is likely why you find that seeking on a local files work (because that's fast) while on a remote it doesn't.
You have at least two demuxers accessing the media resource at the same time here. The behaviour will be very undeterministic (e.g. racey) if you use MediaResource::Read()

So you must use CacheReadAt and ReadAt instead.
So simulate MediaResource::Read by using your own internal offset that is unique to the demuxer/track demuxer.


It's also essential that if the MediaResource length is know, that you will never attempt to read past the end as this would cause the MediaResource to block, and within MSE the MediaResource will never be appended while you're waiting for data causing a dead lock.

If you are to use CacheReadAt, you must pin the resource as there's no guarantee the cache would be expired between the time your retrieve the byte ranges, and the time you actually use them.

I think it would be safe for you to pin the resource, retrieve the cached ranges from the MediaResource and use ReadAt at all times and only use the don't bother with cached reads. 
Something like:
  AutoPinned<MediaResource> resource(mDecoder->GetResource());
  nsTArray<MediaByteRange> ranges;
  nsresult rv = resource->GetCachedRanges(ranges);
  ...
  rv = mResource->ReadAt(mOfffset, buffer, toRead, &bytesRead);
  if (NS_FAILED(rv)) {
    ...
  }

::: dom/media/webm/WebMDemuxer.cpp
@@ +46,5 @@
> +
> +  char *p = static_cast<char *>(aBuffer);
> +  while (NS_SUCCEEDED(rv) && aLength > 0) {
> +    uint32_t bytes = 0;
> +    rv = resource->Read(p, aLength, &bytes);

same command as with WebMBufferedParser.cpp

You'll need to pass to your webm_read your context so you can use MediaResource::ReadAt instead

@@ +153,5 @@
> +nsRefPtr<WebMDemuxer::InitPromise>
> +WebMDemuxer::Init()
> +{
> +  if (ReadMetadata() != NS_OK) {
> +    return InitPromise::CreateAndReject(DemuxerFailureReason::DEMUXER_ERROR, __func__);

If you don't yet have a full metadata, you must return WAITING_FOR_DATA as otherwise the error will be fatal.

You're very likely to see WebMDemuxer::Init called while the MediaResource is empty ; it happens often

@@ +174,5 @@
> +WebMDemuxer::Clone() const
> +{
> +  nsRefPtr<WebMDemuxer> demuxer = new WebMDemuxer(mResource);
> +  demuxer->mBufferedState = mBufferedState;
> +  demuxer->mContext = mContext;

This isn't going to work.

A cloned demuxer will be accessing the MediaResource at the same time ; so if you share the context, it must be thread safe.
I have serious doubt that nestegg is. Starting with reads. You'll get two demuxers sharing the same nestegg context to access two different network location at the same time.

You should consider a clone demuxer a totally separate entity to the first one. I recommend that you only extract the metadata in the original demuxer and pass that to the clone and let the clone do everything from scratch, regardless of what the other demuxer does: so new nestegg context etc...

If you don't want to do that, you must ensure that your context is thread safe, and you'll have to use monitors.

@@ +203,5 @@
> +    return 1;
> +  } else if (aType == TrackInfo::kVideoTrack && mHasVideo) {
> +    return 1;
> +  }
> +  return 0;

I prefer switch case statement. Easier to read and to maintain. We still have plan to support text tracks one day

@@ +262,5 @@
> +  io.userdata = mResource;
> +  int64_t maxOffset = mBufferedState != nullptr ? mBufferedState->GetInitEndOffset() : -1;
> +  int r = nestegg_init(&mContext, io, &webm_log, maxOffset);
> +  if (r == -1) {
> +    return NS_ERROR_FAILURE;

Under which circumstances would mBufferedState not be defined?

nestegg_init will return an error if there are any partial cluster, so it must be run only once we have an init segment, and with maxOffset set to the end of the init segment.

So if mBufferedState->GetInitEndOffset() is > 0, then you need to stop processing and have the init promise reject with WAITING_FOR_DATA.

@@ +337,5 @@
> +      mPicture = pictureRect;
> +      mInitialFrame = frameSize;
> +
> +      switch (params.stereo_mode) {
> +      case NESTEGG_VIDEO_MONO:

nit: two space indents

@@ +479,5 @@
> +
> +  int64_t discardPadding = 0;
> +  (void) nestegg_packet_discard_padding(holder->Packet(), &discardPadding);
> +
> +  // how to handle data chunks vs sample?

Wasn't it assumed that a chunk only ever contained a single sample ? (bug 1157991 and bug 1159593)

@@ +580,5 @@
> +    if (r == -1) {
> +      return nullptr;
> +    }
> +    vpx_codec_stream_info_t si;
> +    memset(&si, 0, sizeof(si));

Use PodZero

@@ +586,5 @@
> +    if (mVideoCodec == NESTEGG_CODEC_VP8) {
> +      vpx_codec_peek_stream_info(vpx_codec_vp8_dx(), data, length, &si);
> +    } else if (mVideoCodec == NESTEGG_CODEC_VP9) {
> +      vpx_codec_peek_stream_info(vpx_codec_vp9_dx(), data, length, &si);
> +    }

side not, seems a bit silly to not have such information stored within the container and having to call the decoder to analyse and retrieve that info

@@ +590,5 @@
> +    }
> +    isKeyframe = si.is_kf;
> +  }
> +
> +  int64_t offset = mResource->Tell();

You can't use that.
Not when you have two demuxers sharing the same resource

@@ +632,5 @@
> +
> +void
> +WebMDemuxer::PushAudioPacket(already_AddRefed<NesteggPacketHolder> aItem)
> +{
> +      mAudioPackets.PushFront(Move(aItem));

nit. 2 spaces indent

@@ +657,5 @@
> +                      target - mSeekPreroll);
> +  }
> +
> +  int r = nestegg_track_seek(mContext, trackToSeek, target);
> +  if (r != 0) {

if (r)

@@ +688,5 @@
> +    uint64_t duration = 0;
> +    if (nestegg_duration(mContext, &duration) == 0) {
> +      buffered +=
> +        media::TimeInterval(media::TimeUnit::FromSeconds(0),
> +                            media::TimeUnit::FromSeconds(duration / NS_PER_S));

Feel free to add a new TimeUnit::FromNanoseconds() method (bonus points for using TimeUnit all across your code)

@@ +787,5 @@
> +already_AddRefed<MediaRawData>
> +WebMTrackDemuxer::NextSample()
> +{
> +  while (mSamples.GetSize() < 1
> +      && mParent->GetNextPacket(mType, &mSamples)) {

&& at the end, and all aligned ; unless you can't make it fit on a 80 characters line

@@ +788,5 @@
> +WebMTrackDemuxer::NextSample()
> +{
> +  while (mSamples.GetSize() < 1
> +      && mParent->GetNextPacket(mType, &mSamples)) {
> +    if (mSamples.GetSize() > 0) {

GetSize() (while it returns in32_t) can never be negative so you only checking if it's != 0, please remove the > 0 instead (I don't entirely agree with this mozilla coding style but that's the way it is)

@@ +803,5 @@
> +  if (!aNumSamples) {
> +    return SamplesPromise::CreateAndReject(DemuxerFailureReason::DEMUXER_ERROR, __func__);
> +  }
> +
> +  while (mSamples.GetSize() < aNumSamples

comparing uint vs int this will cause warnings. Also, you're supposed to return "up to" aNumsamples (or if -1, all the samples available)

@@ +813,5 @@
> +  };
> +
> +  while (aNumSamples > 0 && mSamples.GetSize() > 0) {
> +    samples->mSamples.AppendElement(mSamples.PopFront());
> +    aNumSamples--;

this is very weird.. why do you need two loops here?

@@ +917,5 @@
> +    // There's no next key frame.
> +    *aTime =
> +      media::TimeUnit::FromMicroseconds(std::numeric_limits<int64_t>::max());
> +  } else {
> +    *aTime = mNextKeyframeTime.value();

.ref(), save a copy

::: dom/media/webm/WebMDemuxer.h
@@ +18,5 @@
> +#include "vpx/vpx_codec.h"
> +
> +namespace mozilla {
> +static const unsigned NS_PER_USEC = 1000;
> +static const double NS_PER_S = 1e9;

create TimeUnit::FromNanoseconds instead.. more elegant and we're trying to move to using TimeUnits across the board as it prevents conversion errors (and we've had plenty)

@@ +38,5 @@
> +    }
> +
> +    // We store the timestamp as signed microseconds so that it's easily
> +    // comparable to other timestamps we have in the system.
> +    mTimestamp = timestamp_ns / 1000;

if you define a NS_PER_USEC, you should use it :)

@@ +119,5 @@
> +class MediaRawDataQueue {
> + public:
> +  int32_t GetSize() {
> +    return mQueue.size();
> +  }

why int32_t ? need static_cast if really converting uint32_t -> int32_t

Comment 6

[Jan Gerber]

Attachment: 0001-Bug-1148102-Add-TimeUnit-ToFrames.patch

Comment 7

[Jan Gerber]

Attachment: 0002-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

Comment 8

[Jan Gerber]

Attachment: 0003-Bug-1148102-Add-WebMDemuxer.patch

Comment 9

[Jan Gerber]

Attachment: 0004-Bug-1148102-Hookup-WebMDemuxer.patch

Comment 10

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Please split things in 3 parts:
> - The WebM demuxer
> - The new MediaDataDecoder
> - The integration in the new WebMDecoder and the hook up with
> MediaFormatReader.

Ok new patches splitting it into demuxer/decoder and hookup.


> I'll only look at the WebM demuxer part at this stage.
> 
> ::: dom/media/DecoderTraits.cpp
> @@ +692,4 @@
> >  #endif
> >  #ifdef MOZ_WEBM
> >    if (IsWebMType(aType)) {
> > +    decoderReader = static_cast<MediaDecoderReader*>(new MediaFormatReader(aDecoder, new WebMDemuxer(aDecoder->GetResource())));
> 
> have a preference for that for the time being so we can use the old WebM
> reader until we're happy with the performance of the new one

ok done.

> ::: dom/media/MediaFormatReader.cpp
> @@ +445,5 @@
> > +                                    mAudio.mCallback);
> > +    } else {
> > +      NS_WARNING("Unsupported Audio MimeType");
> > +      return false;
> > +    }
> 
> MediaFormatReader is to be content agnostic. It should never do anything
> related to a particular codec or mime type.
> 
> This should be all handled by the PlatformDecoderModule this is where you
> want to create the appropriate decoder.

ok moved to PlatformDecoder, its a bit more complicated to hook it up there.


> @@ +472,5 @@
> > +                                   mLayersBackendType,
> > +                                   mDecoder->GetImageContainer());
> > +      }
> > +    } else if ((mInfo.mVideo.mMimeType.EqualsLiteral("video/webm; codecs=vp8") ||
> > +             mInfo.mVideo.mMimeType.EqualsLiteral("video/webm; codecs=vp9"))) {
> 
> same as above.
> 
> You also don't want to check the mimetype literally. as there are plenty of
> people using mimetype such as video/webm; codecs=vp8,codecs=vp9 or something
> to that extent.

those are set by the demuxer, its not the string passed by users so should be find (at the new location).


> ::: dom/media/mediasource/MediaSourceReader.cpp
> @@ +700,4 @@
> >  
> >  #ifdef MOZ_WEBM
> >    if (DecoderTraits::IsWebMType(aType)) {
> > +    return static_cast<MediaDecoderReader*>(new MediaFormatReader(aDecoder, new WebMDemuxer(aDecoder->GetResource()), aBorrowedTaskQueue));
> 
> do a similar pref to media.format-reader.mp4 and
> media.mediasource.format-reader.mp4
> 
> so one media.format-reader.webm and media.mediasource.format-reader.webm

done

> 
> ::: dom/media/webm/WebMBufferedParser.cpp
> ... ReadFromCache / Read ...
> ::: dom/media/webm/WebMDemuxer.cpp
> > +    rv = resource->Read(p, aLength, &bytes);

that still needs to be done.

> @@ +153,5 @@
> > +nsRefPtr<WebMDemuxer::InitPromise>
> > +WebMDemuxer::Init()
> > +{
> > +  if (ReadMetadata() != NS_OK) {
> > +    return InitPromise::CreateAndReject(DemuxerFailureReason::DEMUXER_ERROR, __func__);
> 
> If you don't yet have a full metadata, you must return WAITING_FOR_DATA as
> otherwise the error will be fatal.

ok.
Not sure the current MediaFormatReader ever calls Init again if it returns WAITING_FOR_DATA.

> @@ +174,5 @@
> > +WebMDemuxer::Clone() const
> > +{
> > +  nsRefPtr<WebMDemuxer> demuxer = new WebMDemuxer(mResource);
> > +  demuxer->mBufferedState = mBufferedState;
> > +  demuxer->mContext = mContext;
> 
> This isn't going to work.
> 
> A cloned demuxer will be accessing the MediaResource at the same time ; so
> if you share the context, it must be thread safe.
> I have serious doubt that nestegg is. Starting with reads. You'll get two
> demuxers sharing the same nestegg context to access two different network
> location at the same time.
> 
> You should consider a clone demuxer a totally separate entity to the first
> one. I recommend that you only extract the metadata in the original demuxer
> and pass that to the clone and let the clone do everything from scratch,
> regardless of what the other demuxer does: so new nestegg context etc...
> 
> If you don't want to do that, you must ensure that your context is thread
> safe, and you'll have to use monitors.

This still needs to be reworked together with the read calls from above.

> 
> @@ +203,5 @@
> > +    return 1;
> > +  } else if (aType == TrackInfo::kVideoTrack && mHasVideo) {
> > +    return 1;
> > +  }
> > +  return 0;
> 
> I prefer switch case statement. Easier to read and to maintain. We still
> have plan to support text tracks one day

ok done.

> 
> @@ +262,5 @@
> > +  io.userdata = mResource;
> > +  int64_t maxOffset = mBufferedState != nullptr ? mBufferedState->GetInitEndOffset() : -1;
> > +  int r = nestegg_init(&mContext, io, &webm_log, maxOffset);
> > +  if (r == -1) {
> > +    return NS_ERROR_FAILURE;
> 
> Under which circumstances would mBufferedState not be defined?

Init now returns WAITING_FOR_DATA and mBufferedState->GetInitEndOffset() can be used here.

> @@ +337,5 @@
> > +      mPicture = pictureRect;
> > +      mInitialFrame = frameSize;
> > +
> > +      switch (params.stereo_mode) {
> > +      case NESTEGG_VIDEO_MONO:
> 
> nit: two space indents

ok

> @@ +479,5 @@
> > +
> > +  int64_t discardPadding = 0;
> > +  (void) nestegg_packet_discard_padding(holder->Packet(), &discardPadding);
> > +
> > +  // how to handle data chunks vs sample?
> 
> Wasn't it assumed that a chunk only ever contained a single sample ? (bug
> 1157991 and bug 1159593)

ok. can remove loop in the next update.

> @@ +580,5 @@
> > +    if (r == -1) {
> > +      return nullptr;
> > +    }
> > +    vpx_codec_stream_info_t si;
> > +    memset(&si, 0, sizeof(si));
> 
> Use PodZero
> 
ups, missing in current patch. will be in next update.

> @@ +586,5 @@
> > +    if (mVideoCodec == NESTEGG_CODEC_VP8) {
> > +      vpx_codec_peek_stream_info(vpx_codec_vp8_dx(), data, length, &si);
> > +    } else if (mVideoCodec == NESTEGG_CODEC_VP9) {
> > +      vpx_codec_peek_stream_info(vpx_codec_vp9_dx(), data, length, &si);
> > +    }
> 
> side not, seems a bit silly to not have such information stored within the
> container and having to call the decoder to analyse and retrieve that info

indeed, lets discuss this over a beer some time in the future.

> @@ +590,5 @@
> > +    }
> > +    isKeyframe = si.is_kf;
> > +  }
> > +
> > +  int64_t offset = mResource->Tell();
> 
> You can't use that.
> Not when you have two demuxers sharing the same resource

Will go with the read changes from above.

> 
> @@ +632,5 @@
> > +
> > +void
> > +WebMDemuxer::PushAudioPacket(already_AddRefed<NesteggPacketHolder> aItem)
> > +{
> > +      mAudioPackets.PushFront(Move(aItem));
> 
> nit. 2 spaces indent
> 
> @@ +657,5 @@
> > +                      target - mSeekPreroll);
> > +  }
> > +
> > +  int r = nestegg_track_seek(mContext, trackToSeek, target);
> > +  if (r != 0) {
> 
> if (r)

ok

> @@ +688,5 @@
> > +    uint64_t duration = 0;
> > +    if (nestegg_duration(mContext, &duration) == 0) {
> > +      buffered +=
> > +        media::TimeInterval(media::TimeUnit::FromSeconds(0),
> > +                            media::TimeUnit::FromSeconds(duration / NS_PER_S));
> 
> Feel free to add a new TimeUnit::FromNanoseconds() method (bonus points for
> using TimeUnit all across your code)

TimeUnit::FromNanoseconds exists now, added a patch for TimeUnit::ToFrames though.

> @@ +787,5 @@
> > +already_AddRefed<MediaRawData>
> > +WebMTrackDemuxer::NextSample()
> > +{
> > +  while (mSamples.GetSize() < 1
> > +      && mParent->GetNextPacket(mType, &mSamples)) {
> 
> && at the end, and all aligned ; unless you can't make it fit on a 80
> characters line
...
> 
> GetSize() (while it returns in32_t) can never be negative so you only
> checking if it's != 0, please remove the > 0 instead (I don't entirely agree
> with this mozilla coding style but that's the way it is)

ok

> 
> @@ +803,5 @@
> > +  if (!aNumSamples) {
> > +    return SamplesPromise::CreateAndReject(DemuxerFailureReason::DEMUXER_ERROR, __func__);
> > +  }
> > +
> > +  while (mSamples.GetSize() < aNumSamples
> 
> comparing uint vs int this will cause warnings. Also, you're supposed to
> return "up to" aNumsamples (or if -1, all the samples available)
> 
> @@ +813,5 @@
> > +  };
> > +
> > +  while (aNumSamples > 0 && mSamples.GetSize() > 0) {
> > +    samples->mSamples.AppendElement(mSamples.PopFront());
> > +    aNumSamples--;
> 
> this is very weird.. why do you need two loops here?

That was from the assumption that more than one sample could be in a packet. Since its always one the loop is simpler now.

> @@ +917,5 @@
> > +    // There's no next key frame.
> > +    *aTime =
> > +      media::TimeUnit::FromMicroseconds(std::numeric_limits<int64_t>::max());
> > +  } else {
> > +    *aTime = mNextKeyframeTime.value();
> 
> .ref(), save a copy

same code is in MP4Demuxer.cpp, changed to .ref here, but might be good to keep the two in sync.


> @@ +38,5 @@
> > +    }
> > +
> > +    // We store the timestamp as signed microseconds so that it's easily
> > +    // comparable to other timestamps we have in the system.
> > +    mTimestamp = timestamp_ns / 1000;
> 
> if you define a NS_PER_USEC, you should use it :)
> 
> @@ +119,5 @@
> > +class MediaRawDataQueue {
> > + public:
> > +  int32_t GetSize() {
> > +    return mQueue.size();
> > +  }
> 
> why int32_t ? need static_cast if really converting uint32_t -> int32_t

changed to uint32_t, was in line with WebMPacketQueue from WebMReader.h

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624704 [details] [diff] [review]
0001-Bug-1148102-Add-TimeUnit-ToFrames.patch

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

Makes more sense to me to have this in VideoUtils, as we already have UsecsToFrames and FramesToUsecs there.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624707 [details] [diff] [review]
0003-Bug-1148102-Add-WebMDemuxer.patch

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

This is not thread-safe.
mContext will be shared across two threads due to how demuxers are used in MediaFormatReader (for now).
And webmdemux_read will be accessed simultaneously across, giving you wrong reads as MediaResource::Tell() won't get you predictable values.

Don't use MediaResource::Read and instead use MediaReader::ReadAt, it's the only way you can ensure consistent reads.

::: dom/media/webm/WebMBufferedParser.cpp
@@ +351,4 @@
>    }
>  }
>  
> +void WebMBufferedState::UpdateIndex(nsTArray<MediaByteRange>& aRanges, MediaResource* aResource)

you could have simply re-used the existing NotifyDataArrived with something like:
mozilla::UniquePtr<char[]> bytes(new char[aLength]);
NotifyDataArrived(bytes.get(), aLength, aOffset);

prevent having to duplicate everything, nor recheck byte range we've already checked

::: dom/media/webm/WebMDemuxer.h
@@ +142,5 @@
> +  // is responsible for making sure it doesn't get lost.
> +  already_AddRefed<NesteggPacketHolder> DemuxPacket();
> +
> +  // libnestegg context for webm container. Access on state machine thread
> +  // or decoder thread only.

and mainthread (until 1175768 goes in)

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624706 [details] [diff] [review]
0002-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

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

r=me with nit addressed.

Use RefPtr with media taskqueue's tasks (to be consistent with other decoders), and propagate errors to callback.

::: dom/media/platforms/agnostic/OpusDecoder.cpp
@@ +98,5 @@
> +
> +nsresult
> +OpusDecoder::Input(MediaRawData* aSample)
> +{
> +  nsCOMPtr<nsIRunnable> runnable(

RefPtr

@@ +131,5 @@
> +
> +  uint32_t channels = mOpusParser->mChannels;
> +  // No channel mapping for more than 8 channels.
> +  if (channels > 8) {
> +    return -1;

you need to call mCallBack->Error when you have an error.

@@ +152,5 @@
> +    opus_packet_get_samples_per_frame(aData, opus_int32(mOpusParser->mRate));
> +
> +  // A valid Opus packet must be between 2.5 and 120 ms long (48kHz).
> +  int32_t frames = frames_number*samples;
> +  if (frames < 120 || frames > 5760)

missing { }

@@ +269,5 @@
> +
> +nsresult
> +OpusDecoder::Drain()
> +{
> +  nsCOMPtr<nsIRunnable> runnable(

RefPtr

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +44,5 @@
> +    mCodec = Codec::VP9;
> +  } else {
> +    mCodec = -1;
> +  }
> +  memset(&mVPX, 0, sizeof(vpx_codec_ctx_t));

Use PodZero

::: dom/media/platforms/agnostic/VorbisDecoder.cpp
@@ +93,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  r = vorbis_block_init(&mVorbisDsp, &mVorbisBlock);
> +  if (r != 0) {

if (r) { ...

@@ +125,5 @@
> +
> +nsresult
> +VorbisDecoder::Input(MediaRawData* aSample)
> +{
> +  nsCOMPtr<nsIRunnable> runnable(

RefPtr

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624707 [details] [diff] [review]
0003-Bug-1148102-Add-WebMDemuxer.patch

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

::: dom/media/webm/WebMBufferedParser.cpp
@@ +351,4 @@
>    }
>  }
>  
> +void WebMBufferedState::UpdateIndex(nsTArray<MediaByteRange>& aRanges, MediaResource* aResource)

bug 1175768 went in last night.

All of this can be done with with MediaResource::SilentReadAt :

nsRefPtr<MediaByteBuffer> bytes = aResource->SilentReadAt(aOffset, aCount)
WebMBufferedState::NotifyDataArrived(bytes->Elements(), aLength, aOffset);

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: Implement MediaReadAt.

Ensure SilentReadAt seek back to original position even in case of error.
Properly set the MediaByteBuffer length.

This is a convenience method to ReadAt, which doesn't require to loop until we've read all the data and without the overhead of seeking of SilentReadAt.

Comment 16

[Jan Gerber]

Attachment: 0002-Bug-1148102-Add-TimeToFrames.patch

adding TimeToFrames instead of TimeUnit::ToFrames

Comment 17

[Jan Gerber]

Attachment: 0003-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

addressing your review points + renaming Opus and Vorbis decoder classes to avoid clash with names introduced in Bug 1104475

Comment 18

[Jan Gerber]

Attachment: 0004-Bug-1148102-Add-WebMDemuxer.patch

Comment 19

[Jan Gerber]

Attachment: 0005-Bug-1148102-Hookup-WebMDemuxer.patch

class names changed

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8627108 [details] [diff] [review]
Implement MediaReadAt.

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

::: dom/media/MediaResource.cpp
@@ +777,5 @@
>      aOffset += bytesRead;
>      aCount -= bytesRead;
>      curr += bytesRead;
>    }
> +  bytes->SetLength(curr - reinterpret_cast<char*>(bytes->Elements()));

Rather than doing two reinterpret_casts, how about just stashing char* start = curr at the top?

Here and elsewhere.

@@ +782,5 @@
> +  return bytes.forget();
> +}
> +
> +already_AddRefed<MediaByteBuffer>
> +ChannelMediaResource::SilentReadAt(int64_t aOffset, uint32_t aCount)

Does this actually do anything different from the version on the superclass at this point? If not, we can just remove it.

@@ +1295,5 @@
>    // Ensures mSize is initialized, if it can be.
>    // mLock must be held when this is called, and mInput must be non-null.
>    void EnsureSizeInitialized();
> +  already_AddRefed<MediaByteBuffer> UnsafeMediaReadAt(
> +                        int64_t aOffset, uint32_t aCount);

This wrapping is weird.

::: dom/media/MediaResource.h
@@ +290,5 @@
>    virtual nsresult ReadAt(int64_t aOffset, char* aBuffer,
>                            uint32_t aCount, uint32_t* aBytes) = 0;
> +  // This method returns nullptr if anything fails.
> +  // Otherwise, it returns an owned buffer.
> +  // MediaReadAt may return less bytes than requested if end of stream is

"fewer bytes"

@@ +297,3 @@
>    {
>      nsRefPtr<MediaByteBuffer> bytes = new MediaByteBuffer();
> +    bool ok = bytes->SetLength(aCount, fallible);

Oh hm! Is the existing SetCapacity call incorrect? If so, we need to make sure that this fix goes into 41. Can you make sure that happens?

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (:bholley) from comment #20)
> > +already_AddRefed<MediaByteBuffer>
> > +ChannelMediaResource::SilentReadAt(int64_t aOffset, uint32_t aCount)
> 
> Does this actually do anything different from the version on the superclass
> at this point? If not, we can just remove it.
> 

looks like it...


> @@ +1295,5 @@
> >    // Ensures mSize is initialized, if it can be.
> >    // mLock must be held when this is called, and mInput must be non-null.
> >    void EnsureSizeInitialized();
> > +  already_AddRefed<MediaByteBuffer> UnsafeMediaReadAt(
> > +                        int64_t aOffset, uint32_t aCount);
> 
> This wrapping is weird.

it's the wrapping used by the existing code, I simply used exactly that ... the return type is also on a the same line here, except the SilentReadAt you added which add a new line ; so I followed that discrepancy too.

> 
> ::: dom/media/MediaResource.h
> @@ +290,5 @@
> >    virtual nsresult ReadAt(int64_t aOffset, char* aBuffer,
> >                            uint32_t aCount, uint32_t* aBytes) = 0;
> > +  // This method returns nullptr if anything fails.
> > +  // Otherwise, it returns an owned buffer.
> > +  // MediaReadAt may return less bytes than requested if end of stream is
> 
> "fewer bytes"

Yes Stannis Baratheon :)

> 
> @@ +297,3 @@
> >    {
> >      nsRefPtr<MediaByteBuffer> bytes = new MediaByteBuffer();
> > +    bool ok = bytes->SetLength(aCount, fallible);
> 
> Oh hm! Is the existing SetCapacity call incorrect? If so, we need to make
> sure that this fix goes into 41. Can you make sure that happens?

Well, SetCapacity is right, except that the reported length was always 0. The current code never used the Length() method to determine the size, but instead used the aLength parameter... So it's safe for now. Just make SilentReadAt dangerous to use.

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627133 [details] [diff] [review]
0002-Bug-1148102-Add-TimeToFrames.patch

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

::: dom/media/VideoUtils.cpp
@@ +35,4 @@
>    return (CheckedInt64(aUsecs) * aRate) / USECS_PER_S;
>  }
>  
> +// Converts from TimeUnit to number of audio frames,

It's not really a conversion

::: dom/media/VideoUtils.h
@@ +138,5 @@
>  CheckedInt64 UsecsToFrames(int64_t aUsecs, uint32_t aRate);
>  
> +// Converts from TimeUnit to number of audio frames,
> +// given the specified audio/video rate (aRate).
> +CheckedInt64 TimeToFrames(media::TimeUnit aTime, uint32_t aRate);

const media::TimeUnit&

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627135 [details] [diff] [review]
0003-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

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

r=me with comments addressed.

::: dom/media/platforms/agnostic/OpusDecoder.cpp
@@ +47,5 @@
> +
> +nsresult
> +OpusDataDecoder::Init()
> +{
> +  if (NS_FAILED(DecodeHeader(mInfo.mCodecSpecificConfig->Elements(), mInfo.mCodecSpecificConfig->Length()))) {

formatting to 80

@@ +100,5 @@
> +
> +nsresult
> +OpusDataDecoder::Input(MediaRawData* aSample)
> +{
> +  RefPtr<nsIRunnable> runnable(

actually, my bad, it appears that all objects inheriting nsIRunnable should be nsCOMPtr<nsIRunnable>

sorry for that

@@ +134,5 @@
> +  uint32_t channels = mOpusParser->mChannels;
> +  // No channel mapping for more than 8 channels.
> +  if (channels > 8) {
> +    mCallback->Error();
> +    return -1;

wouldn't it be more elegant to call mCallback->Error() in the DoDecoder's callers instead?

@@ +174,5 @@
> +  int ret = opus_multistream_decode(mOpusDecoder,
> +                                    aData, aLength,
> +                                    buffer, frames, false);
> +#endif
> +  if (ret < 0)

missing brackets. DoDecode returns an int ; here you return a boolean and missing mCallback->Error()

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +86,5 @@
> +int
> +VPXDecoder::DoDecodeFrame(MediaRawData* aSample)
> +{
> +  vpx_codec_stream_info_t si;
> +  memset(&si, 0, sizeof(si));

PodZero

@@ +95,5 @@
> +    vpx_codec_peek_stream_info(vpx_codec_vp9_dx(), aSample->mData, aSample->mSize, &si);
> +  }
> +
> +  if (vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
> +    return -1;

mCallback->Error()

::: dom/media/platforms/agnostic/VorbisDecoder.cpp
@@ +125,5 @@
> +
> +nsresult
> +VorbisDataDecoder::Input(MediaRawData* aSample)
> +{
> +  RefPtr<nsIRunnable> runnable(

nsCOMPtr<nsIRunnable>

@@ +144,5 @@
> +  }
> +}
> +
> +int
> +VorbisDataDecoder::DoDecode(MediaRawData* aSample)

you never notify the reader that an error occurred via mCallback->Error()..
it too should be done here

Again, I think this would be more easily handled in DoDecode's caller ; but I'm no fuss about that

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627141 [details] [diff] [review]
0005-Bug-1148102-Hookup-WebMDemuxer.patch

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

Did you check that the hardware Intel VP8 still work?

Comment 25

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #24)
> Did you check that the hardware Intel VP8 still work?

I don't have a computer/os to test this right now but since
WMFDecoderModule::SupportsMimeType claims support for vp8/9
it should be used instead of the agnostic VPX decoder.

Comment 26

[Jean-Yves Avenard [:jya]]

sure, but you added some stuff specific to the webm demuxer and used by the decoder in the extra data field.

wouldn't the HW webm decoder need that information somewhere?

I have a new windows 10 laptop with a new i3.. will give it a try

Comment 27

[Jan Gerber]

discard padding is only added / relevant for opus samples.
VPX should have stayed the same. its good to test of cause.

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: Implement MediaReadAt.

Ensure SilentReadAt seek back to original position even in case of error.
Properly set the MediaByteBuffer length.

Carrying r+ and comments.

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627135 [details] [diff] [review]
0003-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

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

this gives me compilation errors, in particular various signed vs unsigned comparison.

conflicts due to media::Microseconds vs Microseconds as defined in PlatformDecoderModule.h, did you add a using media by any chance anywhere?

::: dom/media/platforms/agnostic/OpusDecoder.cpp
@@ +70,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  */
> +
> +  if (mInfo.mRate != mOpusParser->mRate) {

uint32_t vs int comparison

@@ +74,5 @@
> +  if (mInfo.mRate != mOpusParser->mRate) {
> +    LOG(LogLevel::Warning,
> +        ("Invalid Opus header: container and codec rate do not match!"));
> +  }
> +  if (mInfo.mChannels != mOpusParser->mChannels) {

uint32_t vs int comparison

::: dom/media/platforms/agnostic/VorbisDecoder.cpp
@@ +70,5 @@
> +  size_t available = mInfo.mCodecSpecificConfig->Length();
> +  uint8_t *p = mInfo.mCodecSpecificConfig->Elements();
> +  for(int i = 0; i < 3; i++) {
> +    available -= 2;
> +    if (available < 0) {

this will be always true as size_t is unsigned.

you need to test if available is < 2 prior -= 2

@@ +101,5 @@
> +  if (mInfo.mRate != mVorbisDsp.vi->rate) {
> +    LOG(LogLevel::Warning,
> +        ("Invalid Vorbis header: container and codec rate do not match!"));
> +  }
> +  if (mInfo.mChannels != mVorbisDsp.vi->channels) {

uint32_t vs int

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627133 [details] [diff] [review]
0002-Bug-1148102-Add-TimeToFrames.patch

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

::: dom/media/VideoUtils.h
@@ +138,5 @@
>  CheckedInt64 UsecsToFrames(int64_t aUsecs, uint32_t aRate);
>  
> +// Converts from TimeUnit to number of audio frames,
> +// given the specified audio/video rate (aRate).
> +CheckedInt64 TimeToFrames(media::TimeUnit aTime, uint32_t aRate);

please rename to TimeUnitToFrames (for consistency)

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627138 [details] [diff] [review]
0004-Bug-1148102-Add-WebMDemuxer.patch

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

Thanks ....
almost there.

I'm getting deadlocks on some webm video:
http://giant.gfycat.com/DiligentMediumBangeltiger.webm

The deadlock is when it attempts to seek to 0 following a call to Reset(). This holds the decoder's monitor but data can't be downloaded by the main thread as it needs the decoder's monitor.

not sure of the best way to handle this.

Maybe ensure you only seek in the WebMDemuxer to the first data available ? rather than always 0 ?

::: dom/media/webm/WebMDemuxer.cpp
@@ +25,5 @@
> +
> +namespace mozilla {
> +
> +using namespace gfx;
> +using namespace media;

using namespace media::TimeUnit
as it's the only type you are using from media.

due to unified compilation, this breaks compilation elsewhere otherwise due to type conflicts.
(with PlatformDecoderModule in particular)

@@ +43,5 @@
> +  bool eof = false;
> +  char *p = static_cast<char *>(aBuffer);
> +  demuxer->Read(p, aLength, &bytes);
> +  eof = bytes < aLength;
> +  if (!bytes) {

You should check the return value of WebMDemuxer::Read so you can distinguish EOS which may return a size of 0 and genuine errors.

@@ +54,5 @@
> +{
> +  MOZ_ASSERT(aUserData);
> +  WebMDemuxer* demuxer =
> +    reinterpret_cast<WebMDemuxer*>(aUserData);
> +  NS_ASSERTION(demuxer, "user data must be a WebMDemuxer");

see comment below

@@ +64,5 @@
> +{
> +  MOZ_ASSERT(aUserData);
> +  WebMDemuxer* demuxer =
> +    reinterpret_cast<WebMDemuxer*>(aUserData);
> +  NS_ASSERTION(demuxer, "user data must be a WebMDemuxer");

this isn't dynamic_cast, this can only be 0 if aUserData is also nullptr and you're already asserting if that's the case.

@@ +70,5 @@
> +}
> +
> +static void webmdemux_log(nestegg * context,
> +                     unsigned int severity,
> +                     char const * format, ...)

please use mozilla code style.
nestegg* aContext, etc...

@@ +78,5 @@
> +  }
> +
> +  va_list args;
> +  char msg[256];
> +  const char * sevStr;

const char*

@@ +177,5 @@
> +already_AddRefed<MediaDataDemuxer>
> +WebMDemuxer::Clone() const
> +{
> +  nsRefPtr<WebMDemuxer> demuxer = new WebMDemuxer(mResource);
> +  demuxer->Init();

WebMDemuxer::Init() returns a promise ; MediaDataDemuxer::Clone() is to return a demuxer that is already initialised and ready to use.. 
it would be less confusing if you had a sub-init routine that doesn't return a promise and is synchronous (so you can immediately test for errors).

you need to test for errors, just in case (which could in theory happen if the cache got cleared)

@@ +196,5 @@
> +      return mHasAudio ? 1 : 0;
> +      break;
> +    case TrackInfo::kVideoTrack:
> +      return mHasVideo ? 1 : 0;
> +      break;

default: 0
to avoid enumeration warning.

@@ +198,5 @@
> +    case TrackInfo::kVideoTrack:
> +      return mHasVideo ? 1 : 0;
> +      break;
> +  }
> +  return 0;

remove.

@@ +205,5 @@
> +UniquePtr<TrackInfo>
> +WebMDemuxer::GetTrackInfo(TrackInfo::TrackType aType,
> +                          size_t aTrackNumber) const
> +{
> +  if (aType == TrackInfo::kAudioTrack) {

switch / case

@@ +252,5 @@
> +  io.read = webmdemux_read;
> +  io.seek = webmdemux_seek;
> +  io.tell = webmdemux_tell;
> +  io.userdata = this;
> +  int64_t maxOffset = mBufferedState->GetInitEndOffset();

MOZ_ASSERT that maxOffset is != -1.

@@ +264,5 @@
> +  if (r == -1) {
> +    duration = -1;
> +  }
> +
> +  unsigned int ntracks = 0;

It's a bit confusing that you are using unsigned int whenever dealing with libnestegg only to immediately treat it as uint32_t

Several cases like this,

@@ +268,5 @@
> +  unsigned int ntracks = 0;
> +  r = nestegg_track_count(mContext, &ntracks);
> +  if (r == -1) {
> +    Cleanup();
> +    return NS_ERROR_FAILURE;

you don't need to call Cleanup() whenever an error occurs.
Error are fatals, you won't be called again. you already call Cleanup() in the destructor.

@@ +271,5 @@
> +    Cleanup();
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  for (uint32_t track = 0; track < ntracks; ++track) {

as you're using track later with libnestegg it needs to be unsigned int

@@ +291,5 @@
> +        case NESTEGG_CODEC_VP8:
> +          mInfo.mVideo.mMimeType = "video/webm; codecs=vp8";
> +          break;
> +        case NESTEGG_CODEC_VP9:
> +          mInfo.mVideo.mMimeType = "video/webm; codecs=vp9";

default: MOZ_CRASH()

@@ +395,5 @@
> +          mInfo.mAudio.mCodecSpecificConfig->AppendElements(&c[0], 2);
> +        }
> +        mInfo.mAudio.mCodecSpecificConfig->AppendElements(data, length);
> +      }
> +      if (duration > 0) {

so if we have no duration (and as such duration is -1), is it going to be assumed to be 0 or infinity ?
here it will now assumed that duration == 0

@@ +407,5 @@
> +nsresult
> +WebMDemuxer::Read(char* aBuffer, uint32_t aCount, uint32_t * aBytes)
> +{
> +  nsresult rv = mResource->ReadAt(mOffset, aBuffer, aCount, aBytes);
> +  mOffset += *aBytes;

ReadAt may return fewer bytes than requested ; in particular if you go from a cached area to non-cached.

You are expected to loop until you have read the required number of bytes, got an error or *aBytes == 0 upon return.

I suggest you use the MediaReadAt() convenience method I added in the patch here.
This returns a MediaByteBuffer, you can read its length and copy the data into aBuffer and update aBytes accordingly.

@@ +414,5 @@
> +
> +nsresult
> +WebMDemuxer::Seek(int32_t aWhence, int64_t aOffset)
> +{
> +  if (aWhence == 1) {

please use SEEK_END / SEEK_CUR etc...

@@ +420,5 @@
> +  } else if (aWhence == -1) {
> +    if (mResource->GetLength() - aOffset < 0) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    aOffset =  mResource->GetLength() - aOffset;

GetLength() may not be known for all MediaResource, only if the server supports it. In which case -1 is returned.
If that's the case, you need to return a failure as we can't seek from the end.

@@ +444,5 @@
> +
> +void
> +WebMDemuxer::NotifyDataArrived(uint32_t aLength, int64_t aOffset)
> +{
> +  nsRefPtr<MediaByteBuffer> bytes = mResource->SilentReadAt(aOffset, aLength);

Use the MediaReadAt as added in the patch I attached.
It avoids the need for unecessary seeking which can be an heavy operation

@@ +487,5 @@
> +  if (aType == TrackInfo::kAudioTrack) {
> +    nsRefPtr<NesteggPacketHolder> next_holder(NextPacket(aType));
> +    if (next_holder) {
> +      next_tstamp = next_holder->Timestamp();
> +      PushAudioPacket(next_holder.forget());

remove the need for forget

@@ +497,5 @@
> +  } else if (aType == TrackInfo::kVideoTrack) {
> +    nsRefPtr<NesteggPacketHolder> next_holder(NextPacket(aType));
> +    if (next_holder) {
> +      next_tstamp = next_holder->Timestamp();
> +      PushVideoPacket(next_holder.forget());

remove the need for forget()

@@ +525,5 @@
> +    sample->mDuration = next_tstamp - tstamp;
> +    sample->mOffset = 0;
> +    sample->mKeyframe = holder->IsKeyframe();
> +    if (discardPadding) {
> +        uint8_t c[8];

2 spaces indent

@@ +530,5 @@
> +        BigEndian::writeInt64(&c[0], discardPadding);
> +        sample->mExtraData = new MediaByteBuffer;
> +        sample->mExtraData->AppendElements(&c[0], 8);
> +    }
> +    aSamples->Push(sample.forget());

you don't need forget() here.
see definition of MediaRawDataQueue however.

@@ +573,5 @@
> +    }
> +
> +    if (hasOtherType && otherTrack == holder->Track()) {
> +      // Save the packet for when we want these packets
> +      otherPackets.Push(holder.forget());

drop .forget()

@@ +655,5 @@
> +    skipPacketQueue.PushFront(holder.forget());
> +  }
> +
> +  uint32_t size = skipPacketQueue.GetSize();
> +  for (uint32_t i = 0; i < size; ++i) {

while (skipPacketQueue.GetSize()) {

@@ +665,5 @@
> +
> +void
> +WebMDemuxer::PushAudioPacket(already_AddRefed<NesteggPacketHolder> aItem)
> +{
> +  mAudioPackets.PushFront(Move(aItem));

no need for Move() use NesteggPacketHolder*

@@ +685,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (mSeekPreroll) {
> +    target = std::max(uint64_t(mStartTime * NS_PER_USEC),

uint64_t(mStartTime) * NS_PER_USEC

@@ +720,5 @@
> +    uint64_t duration = 0;
> +    if (nestegg_duration(mContext, &duration) == 0) {
> +      buffered +=
> +        TimeInterval(TimeUnit::FromSeconds(0),
> +                            TimeUnit::FromNanoseconds(duration));

indent

@@ +771,5 @@
> +
> +
> +//WebMTrackDemuxer
> +WebMTrackDemuxer::WebMTrackDemuxer(WebMDemuxer* aParent,
> +                                 TrackInfo::TrackType aType,

indent

@@ +867,5 @@
> +    }
> +
> +    uint32_t size = skipSamplesQueue.GetSize();
> +    for (uint32_t i = 0; i < size; ++i) {
> +      mSamples.PushFront(Move(skipSamplesQueue.PopFront()));

while(skipPacketQueue.GetSize()) {
  mSamples.PushFront(skipPacketQueue.PopFront());

@@ +876,5 @@
> +    frameTime = mParent->GetNextKeyframeTime();
> +  }
> +
> +  if (frameTime != -1) {
> +    mNextKeyframeTime.emplace(

this will assert as mNextKeyframeTime is empty following reset() above.
mNextKeyframeTime = Some(...)

@@ +971,5 @@
> +void
> +WebMTrackDemuxer::BreakCycles()
> +{
> +  mParent = nullptr;
> +

remove empty line

::: dom/media/webm/WebMDemuxer.h
@@ +11,5 @@
> +#include "nsTArray.h"
> +#include "MediaDecoderReader.h"
> +#include "MediaDataDemuxer.h"
> +#include "nsAutoRef.h"
> +#include "nestegg/nestegg.h"

you don't need to include this in the public header.

use forward declaration for nestegg as that's the only type you're using here.

@@ +14,5 @@
> +#include "nsAutoRef.h"
> +#include "nestegg/nestegg.h"
> +
> +#define VPX_DONT_DEFINE_STDINT_TYPES
> +#include "vpx/vpx_codec.h"

put this in WebMDemuxer.cpp

@@ +16,5 @@
> +
> +#define VPX_DONT_DEFINE_STDINT_TYPES
> +#include "vpx/vpx_codec.h"
> +
> +#include "WebMReader.h"

don't include WebMReader.h, extract what you need elsewhere ; or better, split WebMReader from NesteggPacketHolder.

the use forward declaration:
class NesteggPacketHolder;

and only include the header in WebMDemuxer.cpp/

Will make the removal of WebMReader easier when time is due.

@@ +28,5 @@
> +  uint32_t GetSize() {
> +    return mQueue.size();
> +  }
> +
> +  void Push(already_AddRefed<MediaRawData> aItem) {

I would define this as Push(MediaRawData* aItem).
That way you don't need to worry about using nsRefPtr::forget() . mQueue taking nsRefPtr<MediaRawData> everything will be automatically handled nicely.

Being a pointer, the Move is unecessary

@@ +32,5 @@
> +  void Push(already_AddRefed<MediaRawData> aItem) {
> +    mQueue.push_back(Move(aItem));
> +  }
> +
> +  void PushFront(already_AddRefed<MediaRawData> aItem) {

same here: MediaRawData* aItem

@@ +52,5 @@
> +private:
> +  std::deque<nsRefPtr<MediaRawData>> mQueue;
> +};
> +
> +class WebMDemuxer;

you don't need this.

@@ +61,5 @@
> +{
> +public:
> +  explicit WebMDemuxer(MediaResource* aResource);
> +
> +  virtual nsRefPtr<InitPromise> Init() override;

drop the virtual keywords.
override only as it makes virtual implicit

@@ +81,5 @@
> +
> +  virtual void NotifyDataArrived(uint32_t aLength, int64_t aOffset) override;
> +
> +  virtual void NotifyDataRemoved() override;
> +

make the following members private.
WebMTrackDemuxer is already friend class

@@ +90,5 @@
> +
> +  nsresult Reset();
> +
> +  // Pushes a packet to the front of the audio packet queue.
> +  virtual void PushAudioPacket(already_AddRefed<NesteggPacketHolder> aItem);

same as MediaRawDataqueue, change prototype to NesteggPacketHolder* aItem
and drop use to forget()

@@ +95,5 @@
> +
> +  // Pushes a packet to the front of the video packet queue.
> +  virtual void PushVideoPacket(already_AddRefed<NesteggPacketHolder> aItem);
> +
> +  int GetVideoCodec();

where are those methods defined / used ?
Seems to be a remnant of WebMReader
please remove.

@@ +101,5 @@
> +  nsIntSize GetInitialFrame();
> +  int64_t GetLastVideoFrameTime();
> +  void SetLastVideoFrameTime(int64_t aFrameTime);
> +  uint64_t GetCodecDelay() { return mCodecDelay; }
> +  virtual media::TimeIntervals GetBuffered();

this doesn't need to be virtual.

I'm not sure I like having the demuxer calculates the buffered ranges ; the theory is that the overall buffered range is the intersection of the video and audio track buffered range.

@@ +107,5 @@
> +  nsresult Read(char* aBuffer, uint32_t aCount, uint32_t * aBytes);
> +  nsresult Seek(int32_t aWhence, int64_t aOffset);
> +  int64_t Tell();
> +
> +protected:

where are you deriving WebMDemuxer class ?
you don't need protected.
All those members should be private.

@@ +124,5 @@
> +  ~WebMDemuxer();
> +
> +  nsresult ReadMetadata();
> +
> +  nsRefPtr<MediaResource> mResource;

I prefer variables members to be defined at the end, following methods.
Or alternatively, have members variable and methods grouped per category with documentation for either the group or per variables.

@@ +144,5 @@
> +  // Internal method that demuxes the next packet from the stream. The caller
> +  // is responsible for making sure it doesn't get lost.
> +  already_AddRefed<NesteggPacketHolder> DemuxPacket();
> +
> +  // libnestegg context for webm container. Access on state machine thread

it's reader's thread only now for main demuxer, or main thread for cloned demuxer

Comment 32

[Jean-Yves Avenard [:jya]]

Bobby, do we still need to hold the decoder's monitor in ResetDecode() ?

This causes deadlocks attempting to play some webm: 

http://giant.gfycat.com/DiligentMediumBangeltiger.webm

ResetDecode() causes the demuxer to seek to 0, which does a ReadAt() which is waiting on data to be downloaded.
While you have MediaDecoder::NotifyBytesDownloaded() blocking on it to updating the playback rate.
So no data ever arrive to complete MediaResource::ReadAt() and boom deadlock

Comment 33

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #32)
> Bobby, do we still need to hold the decoder's monitor in ResetDecode() ?

I don't believe we need to, in general. We'd need to audit such a patch to make sure it doesn't run any code that still depends on the monitor, but at first glance I think it should be ok.

Comment 34

[Jean-Yves Avenard [:jya]]

With bug 1179569, I have no more deadlock playing plain web.

However, enabling the new webm with mediasource, playback doesn't start. Haven't looked into why.

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8627138 [details] [diff] [review]
0004-Bug-1148102-Add-WebMDemuxer.patch

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

::: dom/media/webm/WebMDemuxer.cpp
@@ +241,5 @@
> +  if (mContext) {
> +    nestegg_destroy(mContext);
> +    mContext = nullptr;
> +  }
> +  mBufferedState = nullptr;

This causes assert failure for me as WebMBufferedState doesn't use thread-safe ref-counted. I don't see why not... will submit a change for that.

@@ +406,5 @@
> +
> +nsresult
> +WebMDemuxer::Read(char* aBuffer, uint32_t aCount, uint32_t * aBytes)
> +{
> +  nsresult rv = mResource->ReadAt(mOffset, aBuffer, aCount, aBytes);

This is what caused the deadlock mentioned in comment 34.
If you attempt to read past the end of the mediaresource, it will block which is something you never want to do ; especially with mediasource.

Adding something like:
  int64_t length = mResource->GetLength();
  if (length >= 0 && aCount + mOffset > length) {
    aCount = length - mOffset;
  }

will ensure you never attempt to read past the end of the mediaresource if the resource's length is known.

@@ +420,5 @@
> +  } else if (aWhence == -1) {
> +    if (mResource->GetLength() - aOffset < 0) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    aOffset =  mResource->GetLength() - aOffset;

double space before mResource

Comment 36

[Jean-Yves Avenard [:jya]]

Playing https://www.youtube.com/watch?v=XqLTe8h0-jo with webm activated; I got several stalls due to decoding error:

in the log.
[61927] WARNING: Overflowing picture rect: file /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/MediaData.cpp, line 300
1203519488[1431c6830]: UpdateLocked resolving MediaPromise (148289be0 created at Ensure)
[61927] WARNING: image allocation error.: file /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/platforms/agnostic/VPXDecoder.cpp, line 159

Comment 37

[Jean-Yves Avenard [:jya]]

Attachment: Mark WebMBufferedParser refcounting as thread-safe.

This is required should we use WebMBufferedParser in a MediaTaskQueue as we don't know which actual thread will actually be used.

Comment 38

[Jan Gerber]

Attachment: 0004-Bug-1148102-Split-NesteggPacketHolder-from-WebMReade.patch

Splitting NesteggPacketHolder so it can be used by WebMReader and WebMDemuxer

Comment 39

[Jan Gerber]

Attachment: 0005-Bug-1148102-Add-TimeUnitToFrames-r-jya.patch

Rename to TimeUnitToFrames 

Carrying r+

Comment 40

[Jan Gerber]

Attachment: 0006-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder-r-j.patch

Addressing comments and carrying r+

Comment 41

[Jan Gerber]

Attachment: 0007-Bug-1148102-Add-WebMDemuxer.patch

Comment 42

[Jean-Yves Avenard [:jya]]

Comment on attachment 8629335 [details] [diff] [review]
0007-Bug-1148102-Add-WebMDemuxer.patch

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

Thanks for the update.

However, I stopped when I saw the changes requested in the previous review not implemented, such as MediaRawDataQueue.

::: dom/media/webm/WebMDemuxer.cpp
@@ +46,5 @@
> +  eof = bytes < aLength;
> +  return NS_FAILED(rv) ? -1 : eof ? 0 : 1;
> +}
> +
> +static int webmdemux_seek(int64_t aOffset, int aWhence, void *aUserData)

void* aUserData

@@ +54,5 @@
> +  nsresult rv = demuxer->Seek(aWhence, aOffset);
> +  return NS_SUCCEEDED(rv) ? 0 : -1;
> +}
> +
> +static int64_t webmdemux_tell(void *aUserData)

void* aUserData

@@ +63,5 @@
> +}
> +
> +static void webmdemux_log(nestegg * aContext,
> +                          unsigned int aSeverity,
> +                          char const * aFormat, ...)

Please Mozilla coding style.
type* blah

@@ +153,5 @@
> +
> +nsresult
> +WebMDemuxer::InitBufferedState()
> +{
> +  if(mBufferedState == nullptr) {

if !mBufferedState

@@ +453,5 @@
> +void
> +WebMDemuxer::NotifyDataArrived(uint32_t aLength, int64_t aOffset)
> +{
> +  WEBM_DEBUG("l: %ld offset: %ld", aLength, aOffset);
> +  nsRefPtr<MediaByteBuffer> bytes = mResource->SilentReadAt(aOffset, aLength);

mResource->MediaReadAt(), can assert that aLength == bytes->Length()

@@ +461,5 @@
> +
> +void
> +WebMDemuxer::NotifyDataRemoved()
> +{
> +  WEBM_DEBUG("not implemented");

it needs to be if we want to enable it with MSE...
Can be done in a linked bug, enabling webm with new MSE

@@ +509,5 @@
> +      next_tstamp = next_holder->Timestamp();
> +      PushVideoPacket(next_holder.forget());
> +    } else {
> +      next_tstamp = tstamp;
> +      next_tstamp += tstamp - mLastVideoFrameTime;

this seems weird.

::: dom/media/webm/WebMDemuxer.h
@@ +22,5 @@
> +  uint32_t GetSize() {
> +    return mQueue.size();
> +  }
> +
> +  void Push(already_AddRefed<MediaRawData> aItem) {

please implement the changes requested in the last review !

Comment 43

[Jan Gerber]

Attachment: 0005-Bug-1148102-Split-WebMPacketQueue-use-nsRefPtr.patch

Also move WebMPacketQueue into NesteggPacketHolder.h
and use NesteggPacketHolder* instead of already_AddRefed<NesteggPacketHolder>

Comment 44

[Jan Gerber]

Attachment: 0008-Bug-1148102-Add-WebMDemuxer.patch

Addressing most of your comments with some exceptions, see below:

>@@ +252,5 @@
>> +  io.read = webmdemux_read;
>> +  io.seek = webmdemux_seek;
>> +  io.tell = webmdemux_tell;
>> +  io.userdata = this;
>> +  int64_t maxOffset = mBufferedState->GetInitEndOffset();
> 
>MOZ_ASSERT that maxOffset is != -1.

For http streams this should do a blocking read, and not fail.
calling nestegg_init with -1 will not read beyond the buffer length since webmdemux_read guards against that. Setting it to maxOffset = mResource->GetLength(); to make it clear in this context.


>@@ +876,5 @@
>> +    frameTime = mParent->GetNextKeyframeTime();
>> +  }
>> +
>> +  if (frameTime != -1) {
>> +    mNextKeyframeTime.emplace(
> 
> 
>this will assert as mNextKeyframeTime is empty following reset() above.
>mNextKeyframeTime = Some(...)

This code is identical to the one in MP4Demuxer.cpp:251.
Not sure why it would assert here and not in MP4Demuxer.

>@@ +509,5 @@
>> +      next_tstamp = next_holder->Timestamp();
>> +      PushVideoPacket(next_holder.forget());
>> +    } else {
>> +      next_tstamp = tstamp;
>> +      next_tstamp += tstamp - mLastVideoFrameTime;
> 
>this seems weird.

adapted from dom/media/webm/SoftwareWebMVideoDecoder.cpp:120

    next_tstamp = tstamp;
    next_tstamp += tstamp - mReader->GetLastVideoFrameTime();

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8629672 [details] [diff] [review]
0008-Bug-1148102-Add-WebMDemuxer.patch

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

Only have my iPad at present which doesn't let me add comment inline.

So here they are:
Remove commented code in WebMTrackBuffer::UpdateSamples

Change the prototype to:
void WebMBufferedState::UpdateIndex(const nsTArray<MediaByteRange>& aRanges, MediaResource* aResource)

aRanges isn't modified in the function -> const, and make everything that can be const, const
E.g    const MediaByteRange& range = aRanges[index];

(There's a few functions in WebMDemuxer that can be changed, like media::TimeUnit to const media::TimeUnit&)

aResource doesn't go beyond the scope of this function, as a rule, don't wrap them in nsRefPtr.


Please link to a try run with WebMDemuxer enabled, let me know if you can't and I'll do it tomorrow.

Comment 46

[Jan Gerber]

Attachment: 0008-Bug-1148102-Add-WebMDemuxer-r-jya.patch

Updating patch adding const.

> Please link to a try run with WebMDemuxer enabled, let me know if you can't and I'll do it tomorrow.

Not able to do that, so plase do so tomorrow.

Carrying r+

Comment 47

[Jean-Yves Avenard [:jya]]

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

Comment 48

[Jean-Yves Avenard [:jya]]

Comment on attachment 8629958 [details] [diff] [review]
0008-Bug-1148102-Add-WebMDemuxer-r-jya.patch

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

some nit...

::: dom/media/webm/WebMBufferedParser.cpp
@@ +358,5 @@
> +    int64_t offset = range.mStart;
> +    uint32_t length = range.mEnd - range.mStart;
> +
> +    uint32_t idx = mRangeParsers.IndexOfFirstElementGt(offset - 1);
> +    if (idx == 0 || !(mRangeParsers[idx-1] == offset)) {

!idx || (mRangeParsers[idx-1] != offset)

@@ +377,5 @@
> +        offset += adjust;
> +        length -= uint32_t(adjust);
> +      } else {
> +        mRangeParsers.InsertElementAt(idx, WebMBufferedParser(offset));
> +        if (idx != 0) {

if (idx) {

::: dom/media/webm/WebMDemuxer.cpp
@@ +338,5 @@
> +          break;
> +      }
> +      uint64_t duration = 0;
> +      r = nestegg_duration(mContext, &duration);
> +      if (r == 0) {

if (!r)

@@ +384,5 @@
> +        mInfo.mAudio.mCodecSpecificConfig->AppendElements(data, length);
> +      }
> +      uint64_t duration = 0;
> +      r = nestegg_duration(mContext, &duration);
> +      if (r == 0) {

if (!r)

@@ +719,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    r = nestegg_offset_seek(mContext, offset);
> +    if (r) {

Almost everywhere you specifically test for the result to be -1.

It would be better to be consistent ; either test for non-zero or explicitly test for -1

Comment 49

[Jean-Yves Avenard [:jya]]

Removed unused variables to see how this go:

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

Comment 50

[Jan Gerber]

Attachment: 0006-Bug-1148102-Add-Opus-VPX-Vorbis-MediaDataDecoder-r-j.patch

add #undef LOG to build with --enable-warnings-as-errors on os x

Carrying r+

Comment 51

[Jan Gerber]

Attachment: 0007-Bug-1148102-Add-WebMDemuxer-r-jya.patch

addressing nits and remove unused variables to build with --enable-warnings-as-errors on os x

Carrying r+

Comment 52

[Jean-Yves Avenard [:jya]]

FWIW, webm with the old MSE and WebMReader appears completely broken at present, and regressed by bug 1175768

Comment 53

[Jan Gerber]

dom/media/mediasource/test/test_FrameSelection.html fails with the new code waiting for a resize event that never gets fired.

This seams to be an issue with MediaFormatReader, VPXDecoder sends a frame with the right picture size to Output.

Comment 54

[Jean-Yves Avenard [:jya]]

Playback works quite nicely in YouTube and WebM (using default MSE).

However, I've hit an assert a couple of times:

(lldb) bt
* thread #244: tid = 0x3b38da, 0x0000000106951bc0 XUL`mozilla::WebMBufferedParser::Append(this=0x000000011e5d9b68, aBuffer=0x0000000163200008, aLength=4625626, aMapping=0x000000012d9fc970, aReentrantMonitor=0x000000012d9fc948) + 1968 at WebMBufferedParser.cpp:172, name = 'MediaPl~ack #10', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000106951bc0 XUL`mozilla::WebMBufferedParser::Append(this=0x000000011e5d9b68, aBuffer=0x0000000163200008, aLength=4625626, aMapping=0x000000012d9fc970, aReentrantMonitor=0x000000012d9fc948) + 1968 at WebMBufferedParser.cpp:172
    frame #1: 0x00000001069526b3 XUL`mozilla::WebMBufferedState::NotifyDataArrived(this=0x000000012d9fc940, aBuffer=0x0000000163200008, aLength=4625626, aOffset=235) + 707 at WebMBufferedParser.cpp:331
    frame #2: 0x0000000106952c38 XUL`mozilla::WebMBufferedState::UpdateIndex(this=0x000000012d9fc940, aRanges=0x00000001330f1a38, aResource=0x000000013a1c30e0) + 888 at WebMBufferedParser.cpp:387
    frame #3: 0x0000000106953d11 XUL`mozilla::WebMDemuxer::EnsureUpToDateIndex(this=0x000000012bc2e800) + 241 at WebMDemuxer.cpp:456
    frame #4: 0x0000000106955b87 XUL`mozilla::WebMDemuxer::GetBuffered(this=0x000000012bc2e800) + 71 at WebMDemuxer.cpp:732
    frame #5: 0x0000000106956e16 XUL`mozilla::WebMTrackDemuxer::GetBuffered(this=0x00000001684ff550) + 54 at WebMDemuxer.cpp:970
    frame #6: 0x0000000106661e56 XUL`mozilla::MediaFormatReader::UpdateReceivedNewData(this=0x0000000132a48000, aTrack=kVideoTrack) + 326 at MediaFormatReader.cpp:797
    frame #7: 0x0000000106664dc2 XUL`mozilla::MediaFormatReader::GetBuffered(this=0x0000000132a48000) + 514 at MediaFormatReader.cpp:1415
    frame #8: 0x00000001067f3df4 XUL`mozilla::SourceBufferDecoder::GetBuffered(this=0x0000000131da3700) + 100 at SourceBufferDecoder.cpp:207
    frame #9: 0x00000001067ff6e6 XUL`mozilla::TrackBuffer::UpdateBufferedRanges(this=0x000000013211d800, aByteRange=(mStart = 0, mEnd = 0, mFuzz = 0), aNotifyParent=false) + 326 at TrackBuffer.cpp:311
    frame #10: 0x0000000106810fa2 XUL`mozilla::TrackBuffer::NotifyReaderDataRemoved(this=0x000000011ff94658)::$_8::operator()() const + 98 at TrackBuffer.cpp:364
    frame #11: 0x0000000106810e8c XUL`nsRunnableFunction<mozilla::TrackBuffer::NotifyReaderDataRemoved(mozilla::MediaDecoderReader*)::$_8>::Run(this=0x000000011ff94640) + 28 at nsThreadUtils.h:256
    frame #12: 0x0000000106628789 XUL`mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run(this=0x000000012166e820) + 281 at TaskDispatcher.h:181
    frame #13: 0x000000010674c1fd XUL`mozilla::MediaTaskQueue::Runner::Run(this=0x0000000121699d00) + 605 at MediaTaskQueue.cpp:256
    frame #14: 0x000000010371cf78 XUL`nsThreadPool::Run(this=0x0000000138bb1f80) + 984 at nsThreadPool.cpp:221
    frame #15: 0x000000010371d07c XUL`non-virtual thunk to nsThreadPool::Run(this=0x0000000138bb1f88) + 28 at nsThreadPool.cpp:235
    frame #16: 0x0000000103719a73 XUL`nsThread::ProcessNextEvent(this=0x0000000136aa4f40, aMayWait=false, aResult=0x00000001330f2c5e) + 1971 at nsThread.cpp:849
    frame #17: 0x000000010378bcb7 XUL`NS_ProcessNextEvent(aThread=0x0000000136aa4f40, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #18: 0x0000000103dd17e0 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x00000001347fd900, aDelegate=0x0000000155af4c80) + 656 at MessagePump.cpp:326
    frame #19: 0x0000000103d3e125 XUL`MessageLoop::RunInternal(this=0x0000000155af4c80) + 117 at message_loop.cc:234
    frame #20: 0x0000000103d3e035 XUL`MessageLoop::RunHandler(this=0x0000000155af4c80) + 21 at message_loop.cc:227
    frame #21: 0x0000000103d3dfdd XUL`MessageLoop::Run(this=0x0000000155af4c80) + 45 at message_loop.cc:201
    frame #22: 0x0000000103717ff9 XUL`nsThread::ThreadFunc(aArg=0x0000000136aa4f40) + 329 at nsThread.cpp:360
    frame #23: 0x0000000103371621 libnss3.dylib`_pt_root(arg=0x00000001450f02d0) + 449 at ptthread.c:212
    frame #24: 0x00007fff8a7e8268 libsystem_pthread.dylib`_pthread_body + 131
    frame #25: 0x00007fff8a7e81e5 libsystem_pthread.dylib`_pthread_start + 176
    frame #26: 0x00007fff8a7e641d libsystem_pthread.dylib`thread_start + 13

This is similar to bug 1180881.

Going to investigate test_FrameSelection.html now.

Comment 55

[Jean-Yves Avenard [:jya]]

Oh I see.. this is related to NotifyReaderDataRemoved() which happens when data is removed from the sourcebuffer.

You then reset the WebMBufferedParser.

But that assumes if you reparse the buffer that there's an init cluster present somewhere in the MediaResource. This isn't always going to be true (and most likely not) if any eviction occurred.

In the particular crash above, the offset is 235, which is probably the size of the init cluster missing.

Comment 56

[Jean-Yves Avenard [:jya]]

Maybe have the WebMDemuxer store a copy of the init segment last seen?
this is what the MP4Demuxer is doing for precisely that reason.

Comment 57

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #53)
> dom/media/mediasource/test/test_FrameSelection.html fails with the new code
> waiting for a resize event that never gets fired.
> 
> This seams to be an issue with MediaFormatReader, VPXDecoder sends a frame
> with the right picture size to Output.

with the current MSE, we have multiple sub-readers, one per discontinuity. So here we would have at least two MediaFormatReader, it's not up to it to detect that the resolution has changed, as the reader only ever see a single resolution.

The new MSE works differently, but its not used here (not only we get an updated ContainerParser for webm)

Comment 58

[Jean-Yves Avenard [:jya]]

I think I know what the problem could be.

In the mean time, one of the cause is this:
diff --git a/dom/media/webm/WebMDemuxer.cpp b/dom/media/webm/WebMDemuxer.cpp
index deed56d0..0a66e7f 100644
--- a/dom/media/webm/WebMDemuxer.cpp
+++ b/dom/media/webm/WebMDemuxer.cpp
@@ -118,6 +118,7 @@ WebMDemuxer::WebMDemuxer(MediaResource* aResource)
   , mVideoCodec(-1)
   , mHasVideo(false)
   , mHasAudio(false)
+  , mNeedReIndex(true)
 {
   if (!gNesteggLog) {
     gNesteggLog = PR_NewLogModule("Nestegg");


So we never switch to the new reader as it returns an empty buffered range.

There something to be something wrong with the cache buffered range calculation in the old MSE.

Comment 59

[Jan Gerber]

Attachment: 0007-Bug-1148102-Add-WebMDemuxer-r-jya.patch

only change: Store init segment in Demuxer and pass to buffered state after reset.

Comment 60

[Jean-Yves Avenard [:jya]]

When changing to the 2nd resolution, WebMTrackDemuxer::Reset() is called.
This webm segment has a buffered range of [2, 4.001).

WebMTrackDemuxer::Reset() does a seek to 0s ; which fails (WebMDemuxer::SeekInternal: track_seek for track 0 to 0 failed, r=-1).

I understand that you used the same logic as the MP4Demuxer, however, the mp4_demuxer::Index object that we use internally has a particular behaviour when seeking to 0, it always returns the first frame available.

You will need to change that. I don't know if looking at what the first sample is, or seeking to the buffered.Start(0) value , something like that.

Comment 61

[Jean-Yves Avenard [:jya]]

Ok.. I found the problem.

When the MediaFormatReader is used, the MediaSourceReader sets the SharedDecoderManager.
SharedDecoderManager is an abomination that was designed to use the same video decoder across all sub-readers.

That means that the MediaSourceReader reuse the VPXDecoder created by the first MediaFormatReader, so here it's one originally set to return a 320x240 images.

Even though we later pass it a 160x120 frames, the image it outputs has a resolution set as 320x240. So the resolution change is never detected, and resize is never fired.

Now there's more at play here. It appears seek doesn't work properly.

When we go back to seeking to 1s, the seek time returned by the first reader (buffered ranges of [0, 4.001) is 2s.
Being 2s it causes the MediaSourceReader to switch back to the 2nd reader (buffered ranges of [2, 4.001) , which will return a frame of the wrong resolution.

I leave that to you.

Amend your patches as follow, r+=me.

In the Integrate WebMDemuxer patch:

diff --git a/dom/media/mediasource/MediaSourceReader.cpp b/dom/media/mediasource/MediaSourceReader.cpp
index 0040c11..e879291 100644
--- a/dom/media/mediasource/MediaSourceReader.cpp
+++ b/dom/media/mediasource/MediaSourceReader.cpp
@@ -759,7 +762,10 @@ MediaSourceReader::CreateSubDecoder(const nsACString& aType, int64_t aTimestampO
   reader->DispatchSetStartTime(0);
 
 #ifdef MOZ_FMP4
-  reader->SetSharedDecoderManager(mSharedDecoderManager);
+  if ((aType.LowerCaseEqualsLiteral("video/mp4") ||
+       aType.LowerCaseEqualsLiteral("audio/mp4")) {
+    reader->SetSharedDecoderManager(mSharedDecoderManager);
+  }
 #endif
   reader->Init(nullptr);
 
In the WebMDemuxer patch:

diff --git a/dom/media/webm/WebMDemuxer.cpp b/dom/media/webm/WebMDemuxer.cpp
index deed56d0..0a66e7f 100644
--- a/dom/media/webm/WebMDemuxer.cpp
+++ b/dom/media/webm/WebMDemuxer.cpp
@@ -118,6 +118,7 @@ WebMDemuxer::WebMDemuxer(MediaResource* aResource)
   , mVideoCodec(-1)
   , mHasVideo(false)
   , mHasAudio(false)
+  , mNeedReIndex(true)
 {
   if (!gNesteggLog) {
     gNesteggLog = PR_NewLogModule("Nestegg");

Good luck :)

Comment 62

[Jean-Yves Avenard [:jya]]

Actually, it's all good with those two patches, it's another bug I introduced elsewhere while debugging this.

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

Comment 63

[Jan Gerber]

Attachment: 0007-Bug-1148102-Add-WebMDemuxer-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #61)
> Amend your patches as follow, r+=me.

(In reply to Jean-Yves Avenard [:jya] from comment #62)
> Actually, it's all good with those two patches, it's another bug I
> introduced elsewhere while debugging this.

amended with the 2 changes

Carrying r+

Comment 64

[Jean-Yves Avenard [:jya]]

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

Reworded the commit logs.

Comment 65

[Jean-Yves Avenard [:jya]]

Comment on attachment 8632072 [details] [diff] [review]
0007-Bug-1148102-Add-WebMDemuxer-r-jya.patch

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

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +761,5 @@
>  #ifdef MOZ_FMP4
> +  if (aType.LowerCaseEqualsLiteral("video/mp4") ||
> +       aType.LowerCaseEqualsLiteral("audio/mp4")) {
> +    reader->SetSharedDecoderManager(mSharedDecoderManager);
> +  }

This should be in the " Hookup WebMDemuxer" patch.

Comment 66

[Jean-Yves Avenard [:jya]]

The code doesn't handle well when data is being removed from the mediaresource.

For example with this video (after enabling media.mediasource.format-reader.webm;true):
https://www.youtube.com/watch?v=jEnd8JIMii4

Let it play a few second, then change the resolution to 4K.

More often than not, I get a decoding error.

If not happening change resolution back to another, and back to 4K, repeat. I always get the problem within 5 times.

Comment 67

[Jean-Yves Avenard [:jya]]

Actually, forget my comment in regards to the change of the MediaSourceReader.

There's a method for this already:
PlatformDecoderModule::SupportsSharedDecoders()

it should simply return false for webm

Comment 68

[Jean-Yves Avenard [:jya]]

when changing resolution, I hit this:
In VPXDecoder:97

   if (vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
    return -1;  <--- here
  }

Comment 69

[Jan Gerber]

Attachment: Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

(In reply to Jean-Yves Avenard [:jya] from comment #68)
> when changing resolution, I hit this:
> In VPXDecoder:97
> 
>    if (vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
>     return -1;  <--- here
>   }

the updated patch logs vpx_codec_decode errors and
skips VPX_CODEC_CORRUPT_FRAME errors instead of failing;
with this change, I did not have any issues switching resolutions.

Comment 70

[Jan Gerber]

Attachment: Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

Updates:
- Seek to first buffered position
- Don't try to set next keyframe for audio tracks
  (without this audio was out of sync on youtube in some cases)

Comment 71

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #67)
> Actually, forget my comment in regards to the change of the
> MediaSourceReader.
> 
> There's a method for this already:
> PlatformDecoderModule::SupportsSharedDecoders()
> 
> it should simply return false for webm

return false in SupportsSharedDecoders for agnostic decoders.

Comment 72

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #69)
> Created attachment 8632239 [details] [diff] [review]
> Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #68)
> > when changing resolution, I hit this:
> > In VPXDecoder:97
> > 
> >    if (vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
> >     return -1;  <--- here
> >   }
> 
> the updated patch logs vpx_codec_decode errors and
> skips VPX_CODEC_CORRUPT_FRAME errors instead of failing;
> with this change, I did not have any issues switching resolutions.

What is surprising is that when using a single VPXDecoder for all streams (and all resolutions) I never saw those decoding errors. Only when using a new decoder for a new stream (as it should be).

Any ideas why?
Why is it finding a corrupt frame when using a new decoder?

Comment 73

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #70)
> - Don't try to set next keyframe for audio tracks
>   (without this audio was out of sync on youtube in some cases)

Why would that be ?
Could that simply hide another problem? like its actually incorrectly seeking to the next cluster?

I don't see otherwise how there could be A/V sync problems simply because internally you search for the next keyframe

Comment 74

[Jean-Yves Avenard [:jya]]

Comment on attachment 8632244 [details] [diff] [review]
Bug-1148102-P9.-Hookup-WebMDemuxer-r-jya.patch

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

I wonder how that will fair with Windows as we're going to return true in WMFDecoderModule::SupportsSharedDecoders for vp8/vp9. Do you know anything about that?

Comment 75

[Jean-Yves Avenard [:jya]]

Comment on attachment 8632242 [details] [diff] [review]
Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

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

You can carry the r+ for those minor changes...

::: dom/media/webm/WebMDemuxer.cpp
@@ +914,5 @@
> +  if (buffered.Length()) {
> +    WEBM_DEBUG("Seek to start point: %f", buffered.Start(0).ToSeconds());
> +    mParent->SeekInternal(buffered.Start(0));
> +    SetNextKeyFrameTime();
> +  }

you probably need a else { mNextKeyframeTime.reset(); } there no?

Comment 76

[Jean-Yves Avenard [:jya]]

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

Comment 77

[Jean-Yves Avenard [:jya]]

You can run locally all the test yourself:
./mach web-platform-tests --include=media-source

that;s the W-3 test in try:


./mach mochitest dom/media/mediasource/test
and
./mach mochitest dom/media/test

That's M2 and M3.

Comment 78

[Jean-Yves Avenard [:jya]]

It now passes all the youtube MSE conformance test:
http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2015.html?timestamp=1419893566526

\o/

Comment 79

[Jean-Yves Avenard [:jya]]

I got a deadlock seeking in Youtube.

The MediaFormatReader is calling VPXDecoder::Flush() which is waiting for all decoding task to complete.

However, it's stuck in what appears to be an infinite loop in:
VPXDecoder::DecodeFrame

Here is the backtrace when I paused the process:

(lldb) bt
* thread #538: tid = 0x51d94b, 0x0000000108543a1e XUL`get_free_fb(cm=0x000000015f616280) + 94 at vp9_onyxc_int.h:309, name = 'MediaPD~oder #4'
    frame #0: 0x0000000108543a1e XUL`get_free_fb(cm=0x000000015f616280) + 94 at vp9_onyxc_int.h:309
    frame #1: 0x0000000108543cad XUL`vp9_receive_compressed_data(pbi=0x000000015f614020, size=7321, psource=0x000000013632d328) + 397 at vp9_decoder.c:305
    frame #2: 0x000000010867c97c XUL`frame_worker_hook(arg1=0x00000001004e56c0, arg2=0x0000000000000000) + 60 at vp9_dx_iface.c:329
    frame #3: 0x00000001084d4088 XUL`execute(worker=0x00000001222c8430) + 56 at vp9_thread.c:134
    frame #4: 0x000000010867c0c2 XUL`decode_one(ctx=0x000000013f7b8810, data=0x000000013632d458, data_sz=7321, user_priv=0x0000000000000000, deadline=0) + 370 at vp9_dx_iface.c:499
  * frame #5: 0x000000010867ae86 XUL`decoder_decode(ctx=0x000000013f7b8810, data=0x000000012e823020, data_sz=7321, user_priv=0x0000000000000000, deadline=0) + 1062 at vp9_dx_iface.c:689
    frame #6: 0x000000010867ea2c XUL`vpx_codec_decode(ctx=0x00000001320e9fa8, data=0x000000012e823020, data_sz=7321, user_priv=0x0000000000000000, deadline=0) + 204 at vpx_decoder.c:122
    frame #7: 0x000000010684c4b5 XUL`mozilla::VPXDecoder::DoDecodeFrame(this=0x00000001320e9f80, aSample=0x000000012e8f9cc0) + 325 at VPXDecoder.cpp:96
    frame #8: 0x000000010684ca5f XUL`mozilla::VPXDecoder::DecodeFrame(this=0x00000001320e9f80, aSample=0x000000012e8f9cc0) + 47 at VPXDecoder.cpp:159
    frame #9: 0x000000010685a63c XUL`void nsRunnableMethodArguments<nsRefPtr<mozilla::MediaRawData> >::apply<mozilla::VPXDecoder, void (this=0x0000000122437630, o=0x00000001320e9f80, m=0x000000010684ca30)(mozilla::MediaRawData*)>(mozilla::VPXDecoder*, void (mozilla::VPXDecoder::*)(mozilla::MediaRawData*)) + 172 at nsThreadUtils.h:634
    frame #10: 0x000000010685a49c XUL`nsRunnableMethodImpl<void (mozilla::VPXDecoder::*)(mozilla::MediaRawData*), true, nsRefPtr<mozilla::MediaRawData> >::Run(this=0x0000000122437600) + 140 at nsThreadUtils.h:828
    frame #11: 0x0000000106742add XUL`mozilla::MediaTaskQueue::Runner::Run(this=0x000000012b7fe980) + 605 at MediaTaskQueue.cpp:256
    frame #12: 0x000000010371e2b8 XUL`nsThreadPool::Run(this=0x0000000143514320) + 984 at nsThreadPool.cpp:228
    frame #13: 0x000000010371e3bc XUL`non-virtual thunk to nsThreadPool::Run(this=0x0000000143514328) + 28 at nsThreadPool.cpp:242
    frame #14: 0x000000010371ac53 XUL`nsThread::ProcessNextEvent(this=0x0000000143f4a9a0, aMayWait=false, aResult=0x000000013632dc6e) + 1971 at nsThread.cpp:867
    frame #15: 0x000000010378d337 XUL`NS_ProcessNextEvent(aThread=0x0000000143f4a9a0, aMayWait=false) + 151 at nsThreadUtils.cpp:277
    frame #16: 0x0000000103dd39a0 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000151e4b3c0, aDelegate=0x00000001422ba1a0) + 656 at MessagePump.cpp:326
    frame #17: 0x0000000103d40325 XUL`MessageLoop::RunInternal(this=0x00000001422ba1a0) + 117 at message_loop.cc:234
    frame #18: 0x0000000103d40235 XUL`MessageLoop::RunHandler(this=0x00000001422ba1a0) + 21 at message_loop.cc:227
    frame #19: 0x0000000103d401dd XUL`MessageLoop::Run(this=0x00000001422ba1a0) + 45 at message_loop.cc:201
    frame #20: 0x0000000103718fc9 XUL`nsThread::ThreadFunc(aArg=0x0000000143f4a9a0) + 329 at nsThread.cpp:360
    frame #21: 0x0000000103371621 libnss3.dylib`_pt_root(arg=0x000000014cb712e0) + 449 at ptthread.c:212
    frame #22: 0x00007fff8a7e8268 libsystem_pthread.dylib`_pthread_body + 131
    frame #23: 0x00007fff8a7e81e5 libsystem_pthread.dylib`_pthread_start + 176
    frame #24: 0x00007fff8a7e641d libsystem_pthread.dylib`thread_start + 13


The loop it's stuck into is this:

libvpx/vp9/vp9_dx_iface.c:687


      while (data_start < data_end) {
        const uint32_t frame_size = (uint32_t) (data_end - data_start);
        const vpx_codec_err_t res = decode_one(ctx, &data_start, frame_size,
                                               user_priv, deadline);
        if (res != VPX_CODEC_OK)
          return res;

        // Account for suboptimal termination by the encoder.
        while (data_start < data_end) {
          const uint8_t marker = read_marker(ctx->decrypt_cb,
                                             ctx->decrypt_state, data_start);
          if (marker)
            break;
          ++data_start;
        }
      }

Looks highly suspicious to me, data_start isn't incremented here on each loop. The call to read_marker returns a value != 0, so we exit the loop, data_start is left untouched and upon exiting we re-enter immediately.
infinite-loop.

Comment 80

[Jean-Yves Avenard [:jya]]

Looking into decode_one function.

The content of:
(lldb) print *(FrameWorkerData*)worker->data1
(FrameWorkerData) $16 = {
  pbi = 0x000000015f614020
  data = 0x000000012e823020 "\x86"
  data_end = 0x000000012e823020 "\x86"
  data_size = 7321
  user_priv = 0x0000000000000000
  result = 2
  worker_id = 0
  received_frame = 1
  scratch_buffer = 0x0000000000000000
  scratch_buffer_size = 0
  stats_mutex = (__sig = 1297437784, __opaque = "")
  stats_cond = (__sig = 1129270852, __opaque = "")
  frame_context_ready = 0
  frame_decoded = -1515870811
}

so we have frameworker->data == frameworker->data_end
after the end of this function, data_start is set to data_end, as they are the same, we will loop forever.

Could that libvpx bug been fixed in an new version of libvpx?

Comment 81

[Jean-Yves Avenard [:jya]]

Submitted a patch to libvpx there:
https://chromium-review.googlesource.com/284960

Comment 82

[Jean-Yves Avenard [:jya]]

Running the mochitest:
./mach mochitest dom/media/test/test_buffered.html

I see a lot of:

[14171] WARNING: WebM packet contains more than one sample: file /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/webm/WebMDemuxer.cpp, line 536

Not sure if this is relevant to the failures seen (which are: 
69 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_buffered.html | split.webm: First range start should be media start - got -2, expected +0
70 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_buffered.html | split.webm: First range end should be media end - expected PASS
)

Comment 83

[Jean-Yves Avenard [:jya]]

Comment on attachment 8628038 [details] [diff] [review]
Implement MediaReadAt.

Moving it to its own bug 1182933

Comment 84

[Jan Gerber]

Attachment: Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

Fail on invalid-preskip.webm again.
Make sure opus decoder fails if WebM CodecDelay is not in sync with Opus preSkip value

Carrying r+

Comment 85

[Jan Gerber]

Attachment: Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #82)
> Running the mochitest:
> ./mach mochitest dom/media/test/test_buffered.html
> 
> I see a lot of:
> 
> [14171] WARNING: WebM packet contains more than one sample: file
> /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/webm/WebMDemuxer.cpp,
> line 536

more than one sample per packet is valid so should remove the warning.
it's happening for ./dom/media/test/sine.webm and unrelated to the buffering issues.


> Not sure if this is relevant to the failures seen (which are: 
> 69 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_buffered.html |
> split.webm: First range start should be media start - got -2, expected +0
> 70 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_buffered.html |
> split.webm: First range end should be media end - expected PASS
> )

this failed because split.webm has a startTime of 2 seconds.
Fixed GetBuffered to not shift by startTime and keep the duration.

Carrying r+

Comment 86

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer-r-jya.patch

Also disable shared decoder if using WMFDecoderModule

Carrying r+

Comment 87

[Jan Gerber]

Attachment: Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

only use bytes returned by MediaReadAt on success.

Carrying r+

Comment 88

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #79)
> I got a deadlock seeking in Youtube.

Wondering if its possible to get a test case of the hangs you see.
Youtube has to many variables to test reliably. What platform are you seeing these hangs.

with the latest patches the tests seam to run ok on my test setup, could you push it to try?

Comment 89

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #88)
> (In reply to Jean-Yves Avenard [:jya] from comment #79)
> > I got a deadlock seeking in Youtube.
> 
> Wondering if its possible to get a test case of the hangs you see.
> Youtube has to many variables to test reliably. What platform are you seeing
> these hangs.
> 
> with the latest patches the tests seam to run ok on my test setup, could you
> push it to try?

I do not.

I play in youtube, changing resolution and seeking a lot.

Comment 90

[Jan Gerber]

with mozilla-central i get one test failure right now:

> 10 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_video_in_audio_element.html | Test timed out. - expected PASS
> 11 INFO Test timed out. Remaining tests=seek.webm-1,vp9.webm-2

This looks more like an issue with MediaFormatReader if the source has only 1 video track
and no audio track and gets loaded into an audio element.
In that case OnDemuxerInitDone does not have any active tracks and should fail, i.e.:

diff --git a/dom/media/MediaFormatReader.cpp b/dom/media/MediaFormatReader.cpp
index 90cba23..fb64278 100644
--- a/dom/media/MediaFormatReader.cpp
+++ b/dom/media/MediaFormatReader.cpp
@@ -369,6 +369,11 @@ MediaFormatReader::OnDemuxerInitDone(nsresult)
     MOZ_ASSERT(mAudioTrackDemuxer);
   }
 
+  if (!videoActive && !audioActive) {
+    mMetadataPromise.Reject(ReadMetadataFailureReason::METADATA_ERROR, __func__);
+    return;
+  }
+
   mInitDone = true;
 
   if (!IsWaitingOnCDMResource() && !EnsureDecodersSetup()) {

Comment 91

[Jan Gerber]

Attachment: Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

Fix seeking to startTime in VP9/Opus.
Was also not working in WebMReader (Bug 1034081)

Carrying r+

Comment 92

[Jean-Yves Avenard [:jya]]

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

Comment 93

[Jean-Yves Avenard [:jya]]

Comment on attachment 8632951 [details] [diff] [review]
Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

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

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +95,5 @@
> +
> +  if (vpx_codec_err_t r = vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
> +    LOG("VPX Decode error: %s", vpx_codec_err_to_string(r));
> +    if (r == VPX_CODEC_CORRUPT_FRAME) {
> +      LOG("Skipping corrupt frame.");

do we still need this ?

Comment 94

[Jean-Yves Avenard [:jya]]

I wouldn't worry about the mediasource tests. The old MSE code is going to be disabled soon.
Having said that, it potentially exhibit problems in the plain webm playback.

Comment 95

[Jan Gerber]

Attachment: Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

(In reply to Jean-Yves Avenard [:jya] from comment #93)
> @@ +95,5 @@
> > +
> > +  if (vpx_codec_err_t r = vpx_codec_decode(&mVPX, aSample->mData, aSample->mSize, nullptr, 0)) {
> > +    LOG("VPX Decode error: %s", vpx_codec_err_to_string(r));
> > +    if (r == VPX_CODEC_CORRUPT_FRAME) {
> > +      LOG("Skipping corrupt frame.");
> 
> do we still need this ?

With the demuxer properly starting with keyframes, this is no longer needed.

There was also an issue with Drain in the decoders that is fixed now.

Carrying r+

Comment 96

[Jan Gerber]

Attachment: Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #72)
> Any ideas why?
> Why is it finding a corrupt frame when using a new decoder?

corrupt frame errors happened because the data passed on was not starting with a keyframe.
Somehow the data changed between looking into the data if its a keyframe and reading it
into MediaRawData. Resulting in issues with SkipToNextKeyframe.

Instead of looking into frame data for the keyframe, the demuxer now uses the webm index to
find random access points.

Carrying r+

Comment 97

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #94)
> I wouldn't worry about the mediasource tests. The old MSE code is going to
> be disabled soon.
> Having said that, it potentially exhibit problems in the plain webm playback.

Indeed, I think they are failing if there is no platform decoder available
(I was testing on linux with ffmpeg decoder module enabled).

In case there is no existing PlatformDecoder, we still want to decode WebM.
For that I added a new AgnosticDecoderModule as a last platform decoder
that can only play agnostic formats.

If that looks good, its time for another try build?

Comment 98

[Jean-Yves Avenard [:jya]]

Comment on attachment 8634618 [details] [diff] [review]
Bug-1148102-P9.-Hookup-WebMDemuxer-r-jya.patch

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

r=me with comments addressed.

BTW, in your commit log. Use a dot between the last word and "r="
thanks

::: dom/media/platforms/agnostic/AgnosticDecoderModule.h
@@ +10,5 @@
> +#include "PlatformDecoderModule.h"
> +
> +namespace mozilla {
> +
> +class AgnosticDecoderModule : public PlatformDecoderModule {

It would be much easier to have AgnosticDecoderModule inherit from BlankDecoderModule

You only need to override SupportsMimeType so it always returns false.

BlankDecoderModule::SupportsSharedDecoders can be overridden to returns false (shareddecodermanager is going away very soon anyway) ; or override it in AgnosticDecoderModule.

Comment 99

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #96)
> Created attachment 8634615 [details] [diff] [review]
> Bug-1148102-P8.-Add-WebMDemuxer-r-jya.patch
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #72)
> > Any ideas why?
> > Why is it finding a corrupt frame when using a new decoder?
> 
> corrupt frame errors happened because the data passed on was not starting
> with a keyframe.
> Somehow the data changed between looking into the data if its a keyframe and
> reading it
> into MediaRawData. Resulting in issues with SkipToNextKeyframe.
> 
> Instead of looking into frame data for the keyframe, the demuxer now uses
> the webm index to
> find random access points.

Maybe it would be better to handle this in MediaFormatReader ; so that it will demux until it finds a keyframe, or likely have it first call MediaTrackDemuxer::SkipToNextKeyFrame instead

I can see how that could be a problem with other decoders too.

Comment 100

[Jean-Yves Avenard [:jya]]

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

Comment 101

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer.-r-jya.patch

(In reply to Jean-Yves Avenard [:jya] from comment #98)
> It would be much easier to have AgnosticDecoderModule inherit from
> BlankDecoderModule
> 
> You only need to override SupportsMimeType so it always returns false.

ok.

> BlankDecoderModule::SupportsSharedDecoders can be overridden to returns
> false (shareddecodermanager is going away very soon anyway) ; or override it
> in AgnosticDecoderModule.

overritten in BlankDecoderModule.

Carrying r+

Comment 102

[Jan Gerber]

Attachment: Bug-1148102-P7.-Add-Opus-VPX-Vorbis-MediaDataDecoder.patch

lets try this again:
fix bool/unsigned int comparison causing build failure on windows
and wrap it in #if defined(DEBUG)

Comment 103

[Jean-Yves Avenard [:jya]]

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

Previous run looks quite good, in particular with MSE, it already seems better than it was before.
\o/

Comment 104

[Jean-Yves Avenard [:jya]]

This is on top of the patch enabling the new MSE. As such webm MSE is now disabled.

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

I don't see much the point on trying to get this new webm to work with the old MSE, we can carry this into another bug.

The crashes on android are worrying though.

Jan, got any ideas about those?

Comment 105

[Jean-Yves Avenard [:jya]]

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

Rebased. Renamed changes from P1 to P7.

Clean up #includes in the various decoders.
Remove includes of OggReader.h and other various stuff that shouldn't be in dom/media/platforms.

If this is all green, I'll push.

Can look after the crashes later and start working on enabling MSE in another bug

Comment 106

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer.-r-jya.patch

Addressing some of the Windows failures:
WMFDecoderModule::SupportsMimeType should only claim VP8/9
support if media.webm.intel_decoder.enabled is set.

Comment 107

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #104)

> The crashes on android are worrying though.
> 
> Jan, got any ideas about those?

No idea so far, only happens on Android 4.0 and not 4.2
application crashed [@ libdvm.so + 0x5a67c]
the minidump does not show anything useful to me so far.

Comment 108

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #106)
> Addressing some of the Windows failures:
> WMFDecoderModule::SupportsMimeType should only claim VP8/9
> support if media.webm.intel_decoder.enabled is set.

good catch !

another try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e73d42cbeb7

Fixed Android compilation, updated with the new hookup patch.

Rebasing again, and more headers cleanup.

This time I re-enabled the webm MSE, as obviously it's very useful to see how it would behave.
And enabling those tests once the new MSE is going to be the default, will become close to impossible.

So better test it now.

Comment 109

[Jean-Yves Avenard [:jya]]

Fixed windows compilation from new patch.

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

Comment 110

[Jan Gerber]

Attachment: Bug-1148102-P9.-Hookup-WebMDemuxer.-r-jya.patch

Fix XP failures: return AgnosticDecoderModule also if
existing platform modules fail in Startup()

Activate WebMDemuxer initialization in TrackBuffersManager

Include "Rebasing again, and more headers cleanup."

Comment 111

[Jan Gerber]

Attachment: Bug-1148102-P7.-Hookup-WebMDemuxer.-r-jya.patch

upload the right file

Comment 112

[Jan Gerber]

Attachment: Bug-1148102-P6.-Add-WebMDemuxer-object.-r-jya.patch

The new MSE code requires mTimecode in MediaRawData, set it.

Comment 113

[Jean-Yves Avenard [:jya]]

Comment on attachment 8635439 [details] [diff] [review]
Bug-1148102-P7.-Hookup-WebMDemuxer.-r-jya.patch

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

::: dom/media/MediaFormatReader.cpp
@@ +258,5 @@
>  bool
>  MediaFormatReader::IsSupportedAudioMimeType(const nsACString& aMimeType)
>  {
> +  return mPlatform && (mPlatform->SupportsMimeType(aMimeType) ||
> +    PlatformDecoderModule::AgnosticMimeType(aMimeType));

isn't mPlatform always going to be defined now?

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +10,5 @@
>  #include "MediaSourceDemuxer.h"
>  
> +#ifdef MOZ_WEBM
> +#include "WebMDemuxer.h"
> +#endif

let's keep this out of this file for the time being, we'll take care of this in another bug...

@@ +760,2 @@
>    if (mType.LowerCaseEqualsLiteral("video/webm") || mType.LowerCaseEqualsLiteral("audio/webm")) {
> +    mInputDemuxer = new WebMDemuxer(mCurrentInputBuffer);

same here.

This code won't be ever called at present as only mp4 is ever passed to the TrackBuffersManager (it's rejected in MediaSource.cpp)

Comment 114

[Jean-Yves Avenard [:jya]]

Comment on attachment 8635439 [details] [diff] [review]
Bug-1148102-P7.-Hookup-WebMDemuxer.-r-jya.patch

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

::: dom/media/platforms/PlatformDecoderModule.cpp
@@ +129,2 @@
>    if (m && NS_SUCCEEDED(m->Startup())) {
>      return m.forget();

the problem here is that if we have a genuine failure (e.g. the PDM does exist but doesn't work for some reason) we'll be failing to propagate the error.

Comment 115

[Jean-Yves Avenard [:jya]]

Actually, we would then fail to create the decoder instead for the known codecs... so that should be fine.

try is closed can't push a new version

Comment 116

[Jean-Yves Avenard [:jya]]

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

Comment 117

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dae3b9b3ec1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a535f9fa0da5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b261aa6976
https://hg.mozilla.org/integration/mozilla-inbound/rev/860a9c9c525c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7f85687fab
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a780859dc1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/06340c449def

Comment 118

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/3dae3b9b3ec1
https://hg.mozilla.org/mozilla-central/rev/a535f9fa0da5
https://hg.mozilla.org/mozilla-central/rev/a4b261aa6976
https://hg.mozilla.org/mozilla-central/rev/860a9c9c525c
https://hg.mozilla.org/mozilla-central/rev/9a7f85687fab
https://hg.mozilla.org/mozilla-central/rev/5a780859dc1c
https://hg.mozilla.org/mozilla-central/rev/06340c449def

Comment 119

[kagami]

Comment on attachment 8635442 [details] [diff] [review]
Bug-1148102-P6.-Add-WebMDemuxer-object.-r-jya.patch

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

::: dom/media/webm/WebMDemuxer.cpp
@@ +264,5 @@
> +    if (id == -1) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    int type = nestegg_track_type(mContext, track);
> +    if (type == NESTEGG_TRACK_VIDEO) {

Shouldn't it check for mHasVideo like in WebMReader::RetrieveWebMMetadata? Right now Nightly shows last video track in multi-video-track webms because of this (I assume). Firefox 40.0.3 displays first video track as usual.

Comment 120

[Jean-Yves Avenard [:jya]]

Please open a new bug if there's an incorrect behaviour in the new webmdemuxer. Thanks