1166779 - Rebase MP3 demuxer on MediaDataDemuxer interface

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Eugen Sawin [:esawin]]

Rebase the MP3Demuxer/MP3TrackDemuxer from bug 1093815 on MediaDataDemuxer/MediaTrackDemuxer from bug 1156708 respectively.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

WIP

Comment 2

[Jean-Yves Avenard [:jya]]

In order to support raw MP3 stream with MSE, we need a ContainerParser specialization.
I've described the details in: https://bugzilla.mozilla.org/show_bug.cgi?id=1169212#c2

The three key methods now required are:
IsInitSegmentPresent() returns a bool if there's the start of an init segment
IsMediaSegmentPresent() returns a bool if there's the start of a media segment header
ParseStartAndEndTimestamps() Is called with data and returns a boolean indicating of a full media segment header was found. The other arguments are obsolete and won't be used.

a call to ParseStartAndEndTimestamps() is to update the value of:
ContainerParser::mHasInitData : bool set to true if an init segment has been found (partial or complete).
ContainerParser::mInitData : Is a fallible array containing the init segment (and only that)
ContainerParser::mCompleteByteRange : Contains a MediaByteRange that is null if we have an incomplete media segment, and set to the byte range of the first full media segment found.

Comment 3

[Matthew Gregan [:kinetik]]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> In order to support raw MP3 stream with MSE, we need a ContainerParser
> specialization.
> I've described the details in:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1169212#c2

Thanks!  We'll keep the MSE support separate as that's lower priority at the moment, but I'd like to see it working.  I've filed bug 1169485 for that work.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

Rebased on the Media(Data|Track)Demuxer interfaces. Some (optional) function like NotifyDataArrived and GetBuffered will be implemented in bug 1163667.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

Same patch but with file renaming detection enabled.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8612832 [details] [diff] [review]
0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

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

looks great.
I'd like to see my comments addressed, in particular the blocking read and implementing GetBuffered().

::: dom/media/MP3Demuxer.cpp
@@ +40,5 @@
> +
> +already_AddRefed<MediaDataDemuxer>
> +MP3Demuxer::Clone() const {
> +  nsRefPtr<MP3Demuxer> demuxer = new MP3Demuxer(mSource);
> +  demuxer->Init();

a cloned MediaDataDemuxer is expected to be initialized. 
MP3Demuxer::Init returns a promise, which may not succeed.

MOZ_ASSERT if it's expected to always succeed.

@@ +51,5 @@
> +}
> +
> +uint32_t
> +MP3Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const {
> +  return aType == TrackInfo::kAudioTrack;

bool to uint32_t ? this is very confusing.
return aType == TrackInfo::kAudioTrack ? 1u : 0;

@@ +64,5 @@
> +}
> +
> +bool
> +MP3Demuxer::IsSeekable() const {
> +  return mSource->IsTransportSeekable();

the MDSM tests if the resource is seekable already.
return true;

@@ +69,5 @@
> +}
> +
> +void
> +MP3Demuxer::NotifyDataArrived(uint32_t aLength, int64_t aOffset) {
> +  // TODO

Please do.

NotifyDataRemoved also needs to be implemented (so it can be used with MSE)

@@ +97,3 @@
>    }
> +
> +  mStreamLength = mSource->GetLength();

MediaResource::GetLength() may be changing (either MSE or radio stream). shouldn't cache that value I don't think.
Could re-read it in NotifyDataArrived or NotifyDataRemoved

@@ +143,5 @@
>  }
>  
> +nsRefPtr<MP3TrackDemuxer::SeekPromise>
> +MP3TrackDemuxer::Seek(TimeUnit aTime) {
> +  const TimeUnit seekTime = ForwardSeek(aTime);

what about seeking backward?

@@ +156,5 @@
>      // Quick seek to the beginning of the stream.
>      mOffset = mFirstFrameOffset;
>      mFrameIndex = 0;
>      mParser.FinishParsing();
> +    return TimeUnit::FromMicroseconds(0);

could do return TimeUnit()

@@ +168,1 @@
>                              mSamplesPerSecond / mSamplesPerFrame;

use aTime.ToSeconds() * mSamplesPerSecond / mSamplesPerFrame;

and you don't need time variable

@@ +174,4 @@
>  }
>  
> +TimeUnit
> +MP3TrackDemuxer::ForwardSeek(TimeUnit aTime) {

This is a confusing name... you can rewind with a ForwardSeek ? :)

@@ +222,5 @@
> +}
> +
> +nsRefPtr<MP3TrackDemuxer::SkipAccessPointPromise>
> +MP3TrackDemuxer::SkipToNextRandomAccessPoint(TimeUnit aTimeThreshold) {
> +  // Will not be called for aduio-only resources.

nit typo

@@ +224,5 @@
> +nsRefPtr<MP3TrackDemuxer::SkipAccessPointPromise>
> +MP3TrackDemuxer::SkipToNextRandomAccessPoint(TimeUnit aTimeThreshold) {
> +  // Will not be called for aduio-only resources.
> +  SkipFailureHolder failure(DemuxerFailureReason::DEMUXER_ERROR, 0);
> +  return SkipAccessPointPromise::CreateAndReject(Move(failure), __func__);

why use a temporary here?

@@ +235,5 @@
> +
> +TimeIntervals
> +MP3TrackDemuxer::GetBuffered() {
> +  // TODO
> +  return TimeIntervals();

please do.

@@ +336,5 @@
>    }
>  
> +  UpdateState(aRange);
> +
> +  frame->mTime = Duration(mFrameIndex - 1).ToMicroseconds();

you can return frame with a presentation time of -1 should the duration be unknown.

this will cause problem with the MDSM when attempting to calculate the start time.

(Note that when used with audio/mpeg, MSE source buffer will generate timestamp automatically.)

@@ +380,5 @@
> +  uint32_t read = 0;
> +  const nsresult rv = mSource->ReadAt(aOffset, reinterpret_cast<char*>(aBuffer),
> +                                      static_cast<uint32_t>(aSize), &read);
> +  NS_ENSURE_SUCCESS(rv, 0);
> +  return static_cast<int32_t>(read);

To be usable within MSE, a Read must *never* be blocking should you reach the end of the resource. You need to check the length of the resource and abort the read if the resource length is < aSize + aOffset
If the resource's length is unknown., then it's okay to block there

::: dom/media/MP3Demuxer.h
@@ +11,3 @@
>  
> +namespace mozilla {
> +// TODO: remove mp3 namespace in bug 1168435.

why remove mp3 namespace in that bug? seems unrelated. I see no references anywhere to anything that would require a mp3 namespace

@@ +319,3 @@
>  
>    // Returns the estimated stream duration in microseconds, or -1 if no
>    // estimation available.

nit: a TimeUnit isn't a microseconds. It has no unit per say. comment should be updated

@@ +338,5 @@
> +  nsRefPtr<SeekPromise> Seek(media::TimeUnit aTime) override;
> +  nsRefPtr<SamplesPromise> GetSamples(int32_t aNumSamples = 1) override;
> +  void Reset() override;
> +  nsRefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(
> +      media::TimeUnit aTimeThreshold) override;

nit: 2 spaces indent

@@ +376,5 @@
>    // MPEG frame parser used to detect frames and extract side info.
>    FrameParser mParser;
>  
>    // Current byte offset in the source stream.
> +  int64_t mOffset;

why the signed type?

::: dom/media/MediaResource.h
@@ +170,1 @@
>  

All of this is formatting only and is outside the scope of this patch.
And seeing there's just about every coding style in this file, you're not really making it more consistent :)

should be done in a separate patch

::: dom/media/gtest/TestMP3Demuxer.cpp
@@ +10,4 @@
>  #include "mozilla/ArrayUtils.h"
>  #include "MockMediaResource.h"
>  
> +using namespace mp3;

you don't need a new namespace...

::: dom/media/moz.build
@@ +137,4 @@
>      'MediaTimer.h',
>      'MediaTrack.h',
>      'MediaTrackList.h',
> +    'MP3Decoder.h',

shouldn't this be added in bug 1168374 instead?

@@ +236,4 @@
>      'MediaTimer.cpp',
>      'MediaTrack.cpp',
>      'MediaTrackList.cpp',
> +    'MP3Decoder.cpp',

same

Comment 7

[Eugen Sawin [:esawin]]

Thanks for the feedback! I'll make sure to address it all in the next patch unless noted otherwise below.

> I'd like to see my comments addressed, in particular the blocking read and
> implementing GetBuffered().

MSE compliance and performance improvements will be handled in bug 1169485 and bug 1163667 respectively. If there are no other reasons to have that part of the interface implemented, I would like to land this version asap to get more testing exposure for general playback and seeking in the meanwhile. Currently, there is no working MP3 playback for Android L.

> > +uint32_t
> > +MP3Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const {
> > +  return aType == TrackInfo::kAudioTrack;
> 
> bool to uint32_t ? this is very confusing.
> return aType == TrackInfo::kAudioTrack ? 1u : 0;

It's standard-compliant, but I agree it might look unintuitive.

> > +void
> > +MP3Demuxer::NotifyDataArrived(uint32_t aLength, int64_t aOffset) {
> > +  // TODO
> 
> Please do.
> 
> NotifyDataRemoved also needs to be implemented (so it can be used with MSE)

I would prefer to handle it in the follow-ups, if possible, see first comment.

> > +TimeUnit
> > +MP3TrackDemuxer::ForwardSeek(TimeUnit aTime) {
> 
> This is a confusing name... you can rewind with a ForwardSeek ? :)

Names are difficult :). Its original name was SlowSeek, technically it does only seek forward, except when it doesn't, which is when it calls to FastSeek. I'll figure out a better name.

> > +  UpdateState(aRange);
> > +
> > +  frame->mTime = Duration(mFrameIndex - 1).ToMicroseconds();
> 
> you can return frame with a presentation time of -1 should the duration be
> unknown.
> 
> this will cause problem with the MDSM when attempting to calculate the start
> time.
> 
> (Note that when used with audio/mpeg, MSE source buffer will generate
> timestamp automatically.)

So with MSE we don't need to set the time stamps in the demuxer? What's a better fallback value when we have to set a time stamp but fail to compute one for some reason?

> > +  uint32_t read = 0;
> > +  const nsresult rv = mSource->ReadAt(aOffset, reinterpret_cast<char*>(aBuffer),
> > +                                      static_cast<uint32_t>(aSize), &read);
> > +  NS_ENSURE_SUCCESS(rv, 0);
> > +  return static_cast<int32_t>(read);
> 
> To be usable within MSE, a Read must *never* be blocking should you reach
> the end of the resource. You need to check the length of the resource and
> abort the read if the resource length is < aSize + aOffset
> If the resource's length is unknown., then it's okay to block there

Since we return the read bytes, I think reading to the end of the resource would be preferable in this case instead of aborting the read.
 
> > +namespace mozilla {
> > +// TODO: remove mp3 namespace in bug 1168435.
> 
> why remove mp3 namespace in that bug? seems unrelated. I see no references
> anywhere to anything that would require a mp3 namespace

Sorry about the random comment, the mp3 namespace is there to prevent a name conflict (ID3Parser) with MP3FrameParser in the unified source files. It can be removed once we've replace all uses of MP3FrameParser in bug 1168435.

> >    // Current byte offset in the source stream.
> > +  int64_t mOffset;
> 
> why the signed type?

To be consistent with MediaResource and MediaByteRange.

> ::: dom/media/MediaResource.h
> @@ +170,1 @@
> >  
> 
> All of this is formatting only and is outside the scope of this patch.
> And seeing there's just about every coding style in this file, you're not
> really making it more consistent :)

I've touched the file to make it const-correct (my patch wouldn't compile otherwise), the formatting fixes were just opportunistic. It may not be fully consistent yet, but it's a start.

Thanks for the great feedback again, and Matthew, feel free to redirect the r? if you're OK with that you've seen.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

Patch with feedback comments addressed.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

Ref pointer fix.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8613173 [details] [diff] [review]
0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +60,5 @@
> +}
> +
> +uint32_t
> +MP3Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const {
> +  return aType == TrackInfo::kAudioTrack;

still there ? :)

@@ +133,5 @@
> +  mInfo->mRate = mSamplesPerSecond;
> +  mInfo->mChannels = mChannels;
> +  mInfo->mBitDepth = 16;
> +  mInfo->mMimeType = "audio/mpeg";
> +  mInfo->mDuration = Duration().ToMicroseconds();

the default MediaInfo initializer will set mTrackID to TRACK_INVALID, you should set up a trackID value there. Any will do. just not invalid

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #7)

> So with MSE we don't need to set the time stamps in the demuxer? What's a
> better fallback value when we have to set a time stamp but fail to compute
> one for some reason?

Forgot to answer that one. This is an important question, and something that must be resolved when not using MSE as if a presentation time of -1 is passed to the MediaDecoderStateMachine it will trigger an assert sooner a later and will prevent seeking to work properly.

Per MSE spec, http://w3c.github.io/media-source/index.html#sourcebuffer-generate-timestamps-flag
this flag is set to true for the "audio/mpeg" and "audio/aac" mimetype (http://w3c.github.io/media-source/byte-stream-format-registry.html). When this flag is set, the MSE "segment parser loop" will totally ignore the presentation and decode timestamp and will set them.

Outside of MSE, it is expected that timestamps are monotonically increasing, the start time is set on the presentation timestamp read from the first demuxed sample, that first value is then use as an offset to make the first sample have a presentation time of 0. For simplicity sake I suggest you start with a value of 0.

When seeking, simply set the timestamp to the seek time. Each time a sample is demuxed, increase the timestamp by the sample duration.

Comment 12

[Eugen Sawin [:esawin]]

> > +uint32_t
> > +MP3Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const {
> > +  return aType == TrackInfo::kAudioTrack;
> 
> still there ? :)

Sorry, it slipped through into the wrong commit (https://github.com/eamsen/gecko-dev/commit/2e7b8c56e98c8a672243d939af1294e6ad97fb53).

> the default MediaInfo initializer will set mTrackID to TRACK_INVALID, you
> should set up a trackID value there. Any will do. just not invalid

The default AudioInfo constructor initializes it to 1.

Re comment 11: So we're already doing the right thing for the MSE and non-MSE case regarding the time stamps, except that there is a possibility of us passing -1 in unexpected conditions. I don't see how we could assemble anything meaningful in such a case, so I will just assert that it never happens. The monotonicity follows from the way we derive the duration, but I could add some tracking to that and assert that the behaviour is as expected.

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8613173 [details] [diff] [review]
0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

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

Looks good to me.  Sorry for the review delay, public holiday and then a sick day here.

::: dom/media/MP3Demuxer.cpp
@@ +26,5 @@
> +  : mSource(aSource)
> +{}
> +
> +bool
> +MP3Demuxer::_Init() {

Pretty sure identifiers starting with an underscore and an uppercase letter are reserved in C++.  Rename please? InitIniternal(), DoInit() or something?

@@ +78,5 @@
> +}
> +
> +void
> +MP3Demuxer::NotifyDataArrived(uint32_t aLength, int64_t aOffset) {
> +  // TODO

For all of the MSE todos, reference the bug and stick an NS_WARNING("Unimplemented function foo") in for now.

@@ +111,5 @@
>  }
>  
> +void
> +MP3TrackDemuxer::UpdateResourceStats() {
> +  mStreamLen = mSource->GetLength();

Why do we want to cache this at all?  Can StreamLength() simply return the result of mSource->GetLength()?  Then we could do away with the UpdateResourceStats() stuff altogether.

@@ +118,5 @@
>  bool
> +MP3TrackDemuxer::Init() {
> +  UpdateResourceStats();
> +  FastSeek(TimeUnit());
> +  nsRefPtr<MediaRawData> frame(GetNextFrame(FindNextFrame()));

Include a comment here explaining that we need to force the first frame to be parsed to fetch the sample rate etc. and seek back to the beginning to avoid dropping that packet.

@@ +399,5 @@
>  }
>  
> +int32_t
> +MP3TrackDemuxer::Read(uint8_t* aBuffer, int64_t aOffset, int32_t aSize) {
> +  aSize = std::min(static_cast<int64_t>(aSize), StreamLength() - aOffset);

std::min<int64_t>(..., ...) is slightly nicer.

::: dom/media/MP3Demuxer.h
@@ +355,5 @@
>    // Fast approximate seeking to given time.
> +  media::TimeUnit FastSeek(media::TimeUnit aTime);
> +
> +  // Seeking by reading the stream up to the given time for more accurate results.
> +  media::TimeUnit ForwardSeek(media::TimeUnit aTime);

Maybe rename this to ScanForward(), ScanUntil() or something, since it's not (exactly) seeking.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

Addressed review comments.

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8614935 [details] [diff] [review]
0001-Bug-1166779-Rebase-MP3-demuxer-on-MediaDataDemuxer-i.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +255,5 @@
>  }
>  
>  int64_t
> +MP3TrackDemuxer::GetEvictionOffset(TimeUnit aTime) {
> +  return -1;

I should have made that a uint64_t.. you don't want to return a negative number here. 0 means nothing can be evicted.

not that it matters, I will be removing that API.

Comment 16

[Eugen Sawin [:esawin]]

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89693ae900b7

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/89693ae900b7