Closed Bug 1156708 Opened 5 years ago Closed 5 years ago

Add MediaFormatDecoder object

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(6 files, 11 obsolete files)

1.42 KB, patch
ajones
: review+
Details | Diff | Splinter Review
2.44 KB, patch
Details | Diff | Splinter Review
78.10 KB, patch
Details | Diff | Splinter Review
4.89 KB, patch
Details | Diff | Splinter Review
2.20 KB, patch
Details | Diff | Splinter Review
24.16 KB, patch
Details | Diff | Splinter Review
A MediaFormatDecoder provides backward compatibility with a MediaDecoderReader but works with the new architecture as described in bug 1148292.

It will take in its constructor a MediaDataDemuxer and will manage a PlatformDecoderModule and its associated MediaDataDecoder.


Name could change.
Depends on: 1157075
Add SkipToNextRandomAccessPoint API.
Attachment #8602715 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Add preferences to turn on the new code. One is for plain mp4, the other one for MP4 within mse.
Attachment #8603123 - Flags: review?(ajones)
Comment on attachment 8602715 [details] [diff] [review]
Part2. Add SkipToNextRandomAccessPoint

obviously wrong bug # :(
Attachment #8602715 - Attachment is obsolete: true
Attachment #8602715 - Flags: review?(cpearce)
Comment on attachment 8603123 [details] [diff] [review]
Part2. Add pref to toggle new code

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

Please add a default pref into all.js
Attachment #8603123 - Flags: review?(ajones) → review+
Carrying r+. As per IRC, renaming to media.mediasource.format-decoder.mp4=true and media.format-decoder.mp4=true. Adding those in all.js
Attachment #8603123 - Attachment is obsolete: true
Disable audio track when using combined audio + video sourcebuffer
Attachment #8603894 - Flags: review?(ajones)
Attached patch Part 4. Disable test (obsolete) — Splinter Review
Disable tests as we can't have them give consistent results. When using the MP4Reader (soon to be obsolete) those tests will fail. But using the new MediaFormatDecoder they pass (yeah\!). Will re-enable those tests once MP4Reader is retired.
Attachment #8603902 - Flags: review?(karlt)
Attachment #8603894 - Flags: review?(ajones) → review+
Comment on attachment 8603902 [details] [diff] [review]
Part 4. Disable test

Given the new code is not enabled yet, there is no need to change anything here yet, right?
Any changes can be made when the pref change is made.

Further, the results will only change on some platforms; the tests on other platforms are already passing (because the results of the only subtest are ignored).

Please keep the reference to bug 1154881, because changing the code won't make the test correct.
Attachment #8603902 - Flags: review?(karlt)
(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> Comment on attachment 8603902 [details] [diff] [review]
> Part 4. Disable test
> 
> Given the new code is not enabled yet, there is no need to change anything
> here yet, right?
> Any changes can be made when the pref change is made.

Correct.. However as part of my try run, I do enable those prefs. This causes oranges as we pass all those tests now.

> 
> Further, the results will only change on some platforms; the tests on other
> platforms are already passing (because the results of the only subtest are
> ignored).

Yes, actually none of those changes are required now. It should behave the same on all platforms.

> 
> Please keep the reference to bug 1154881, because changing the code won't
> make the test correct.
The big one... https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc03b854c27  all green, and amazingly enough, reliably so. I am addressing the build error on B2G L in bug 1163458
Attachment #8603951 - Flags: review?(cpearce)
Depends on: 1163485
Blocks: 1163485
No longer depends on: 1163485
Comment on attachment 8603902 [details] [diff] [review]
Part 4. Disable test

moving it to bug 1163485
Attachment #8603902 - Attachment is obsolete: true
mInitDone was set in the wrong place. It could cause crashes in MediaFormatDecoder::NotifyDataArrived which would attempt to use mMainThreadDemuxer.
Attachment #8604407 - Flags: review?(cpearce)
Attachment #8604407 - Flags: review?(cpearce) → review+
Comment on attachment 8603951 [details] [diff] [review]
Part1. Add MediaFormatDecoder player

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

::: dom/media/MediaFormatDecoder.cpp
@@ +46,5 @@
> +
> +namespace mozilla {
> +
> +// Uncomment to enable verbose per-sample logging.
> +//#define LOG_SAMPLE_DECODE 1

We should use NSPR_LOG_LEVEL=6 (on the command line) here instead of having to uncomment and recompile here. JW just landed https://hg.mozilla.org/integration/mozilla-inbound/rev/21401957b641 to make this change elsewhere.

@@ +90,5 @@
> +
> +MediaFormatDecoder::~MediaFormatDecoder()
> +{
> +  MOZ_COUNT_DTOR(MediaFormatDecoder);
> +  // shutdown main thread demuxer and track demuxers.

If these are "main thread" demuxers, should they be shutdown only on the main thread?

Longer term, I kind of feel like we should split the responsibility of calculating the buffered ranges out into a separate object, since it is called on different threads.

@@ +122,5 @@
> +    mAudio.mTrackDemuxer = nullptr;
> +  }
> +  if (mAudio.mTaskQueue) {
> +    mAudio.mTaskQueue->BeginShutdown();
> +    mAudio.mTaskQueue->AwaitShutdownAndIdle();

It's a shame we have to block here waiting for AwaitShutdownAndIdle() to finish. My kingdom for a Promises.all() implementation!

@@ +525,5 @@
> +    return VideoDataPromise::CreateAndReject(DECODE_ERROR, __func__);
> +  }
> +  if (IsSeeking()) {
> +    VLOG("called mid-seek. Rejecting.");
> +    return VideoDataPromise::CreateAndReject(CANCELED, __func__);

Should this be a MOZ_DIAGNOSTIC_ASSERT? This should only happen if the caller is broken, right?

@@ +536,5 @@
> +    NS_WARNING("Error constructing decoders");
> +    return VideoDataPromise::CreateAndReject(DECODE_ERROR, __func__);
> +  }
> +
> +  if (mShutdown) {

Shouldn't the mShutdown check come before we call EnsureDecodersSetup()? There doesn't seem to be much point in creating a decoder if we've already shutdown.

@@ +635,5 @@
> +    NS_WARNING("Error constructing decoders");
> +    return AudioDataPromise::CreateAndReject(DECODE_ERROR, __func__);
> +  }
> +
> +  if (mShutdown) {

mShutdown check before EnsureDecodersSetup()?

@@ +700,5 @@
> +}
> +
> +void
> +MediaFormatDecoder::Update(TrackType aTrack)
> +{

This function is too long, it's hard to read. Can you factor it up to call sub-functions for the logical parts?

@@ +738,5 @@
> +        MOZ_ASSERT(!decoder.HasPromise());
> +        VLOG("We have new data. Resolving WaitingPromise");
> +        MediaData::Type type = aTrack == TrackInfo::kVideoTrack ?
> +          MediaData::VIDEO_DATA : MediaData::AUDIO_DATA;
> +        decoder.mWaitingPromise.Resolve(type, __func__);

Can't you use:

decoder.mWaitingPromise.Resolve(decoder.mType, __func__);

?

::: dom/media/MediaFormatDecoder.h
@@ +29,5 @@
> +#else
> +#undef READER_DORMANT_HEURISTIC
> +#endif
> +
> +class MediaFormatDecoder final : public MediaDecoderReader

Can we call this a "Reader", i.e. MediaFormatReader, so that it's clear from its name that it inherits from MediaDecoderReader and not MediaDecoder?

@@ +156,5 @@
> +      , mType(aType)
> +    {
> +    }
> +    virtual void Output(MediaData* aSample) override {
> +      mReader->Output(mType, aSample);

If the DecoderCallback callbacks stuck tasks on the decode task queue to call the Reader, instead of calling directly on whatever taskqueue/thread the MediaDataDecoder calls it in, you wouldn't need DecoderData::mMonitor anymore.
Attachment #8603951 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #13)

> > +  MOZ_COUNT_DTOR(MediaFormatDecoder);
> > +  // shutdown main thread demuxer and track demuxers.
> 
> If these are "main thread" demuxers, should they be shutdown only on the
> main thread?

They are main thread in the sense that they are only used by the main thread, but we could delete them anywhere so long as we've stopped using them.

At first I had done so in Shutdown(), but it turns out that MediaDecoderReader::NotifyDataArrived can be called after Shutdown() or just about the same time.
I couldn't even rely on mShutdown being set as I had crashes where MDR::NotifyDataArrived was called right in between the time MediaFormatDecoder::Shutdown() was called and the time MediaDecoderReader::Shutdown (which set mShutdown to true).

Not wanting to introduce another monitor for this situation, I did so in the destructor instead.

> 
> Longer term, I kind of feel like we should split the responsibility of
> calculating the buffered ranges out into a separate object, since it is
> called on different threads.

This is what I've done. You have a demuxer used to calculate the buffered range on the main thread, and one used during playback.

There's still the issue where the MDSM calls GetBuffered() which will force us to keep the lock unfortunately.

> 
> @@ +122,5 @@
> > +    mAudio.mTrackDemuxer = nullptr;
> > +  }
> > +  if (mAudio.mTaskQueue) {
> > +    mAudio.mTaskQueue->BeginShutdown();
> > +    mAudio.mTaskQueue->AwaitShutdownAndIdle();
> 
> It's a shame we have to block here waiting for AwaitShutdownAndIdle() to
> finish. My kingdom for a Promises.all() implementation!
> 

Shutdown() is one aspect I didn't dig into. However, now that multiple readers are becoming a thing of the past with MSE, it's not going to cause as much problem as we have now.
 
> @@ +525,5 @@
> > +    return VideoDataPromise::CreateAndReject(DECODE_ERROR, __func__);
> > +  }
> > +  if (IsSeeking()) {
> > +    VLOG("called mid-seek. Rejecting.");
> > +    return VideoDataPromise::CreateAndReject(CANCELED, __func__);
> 
> Should this be a MOZ_DIAGNOSTIC_ASSERT? This should only happen if the
> caller is broken, right?

Actually, we have a MOZ_DIAGNOSTIC_ASSERT(mSeekPromise.IsEmpty()) just above, which will be equivalent to testing IsSeeking(), this is a redundant test. But just in case. I'll made it an assert as well

> 
> @@ +536,5 @@
> > +    NS_WARNING("Error constructing decoders");
> > +    return VideoDataPromise::CreateAndReject(DECODE_ERROR, __func__);
> > +  }
> > +
> > +  if (mShutdown) {
> 
> Shouldn't the mShutdown check come before we call EnsureDecodersSetup()?
> There doesn't seem to be much point in creating a decoder if we've already
> shutdown.

indeed. same issue in MP4Reader.

> > +void
> > +MediaFormatDecoder::Update(TrackType aTrack)
> > +{
> 
> This function is too long, it's hard to read. Can you factor it up to call
> sub-functions for the logical parts?

I'll see what I can come up with.
> > +        MediaData::Type type = aTrack == TrackInfo::kVideoTrack ?
> > +          MediaData::VIDEO_DATA : MediaData::AUDIO_DATA;
> > +        decoder.mWaitingPromise.Resolve(type, __func__);
> 
> Can't you use:
> 
> decoder.mWaitingPromise.Resolve(decoder.mType, __func__);

yes !

> > +class MediaFormatDecoder final : public MediaDecoderReader
> 
> Can we call this a "Reader", i.e. MediaFormatReader, so that it's clear from
> its name that it inherits from MediaDecoderReader and not MediaDecoder?

I have to admit I was greatly influenced by the AVFormatDecoder found in ffmpeg and mythtv :)

I personally dislike our existing naming very much.
I see it as having different "decoder", a format decoder, a data decoder etc...

rather than a format reader, and a data decoder. 

But if you want it.. sure

> If the DecoderCallback callbacks stuck tasks on the decode task queue to
> call the Reader, instead of calling directly on whatever taskqueue/thread
> the MediaDataDecoder calls it in, you wouldn't need DecoderData::mMonitor
> anymore.

There is still the case of MDSM calling GetBuffered and having to keep a monitor while it reads the cached buffered range.
I didn't want to have a 3 demuxers for that particular case.

In practice we wouldn't need a lock even if the MDSM calls GetBuffered() on its own thread, because it only ever does so right after Request*Data completes and before requesting new data. So by the time the MDSM calls GetBuffered() there's never any tasks that could run that would update mTimeRanges.

But this would make for a very difficult read and the next maintainer would be puzzled for sure.

thanks for the review !
Carrying r+, changing log system (tidying them up while at it). Split Update functions into three. Renaming into MediaFormatReader
Attachment #8603951 - Attachment is obsolete: true
Rebasing following rename of class. Also renaming pref to media.*.format-reader.*
Attachment #8603147 - Attachment is obsolete: true
Attachment #8604407 - Attachment is obsolete: true
Attached patch Remove locking to some members (obsolete) — Splinter Review
As discussed earlier, this remove the need for a lock between the reader and decoder thread. As the MDSM keeps checking on the size of our audio and video queue we still need a lock there. Also, I didn't find a way to get rid of the use of mIsFlushing due to how Flush() works. So we still need the monitor there. I have devised some methods but all of them require more work than a monitor, for no apparent benefit. And a few improvements here and there (little code reorganisation, and simplifying the waiting for data).
Attachment #8606606 - Flags: review?(cpearce)
This is an adaptation of bug 1161984. I had already got rid of IsWaitingMediaResources(). I will merge the patches related to MediaFormatReader together before pushing to inbound. Reworking the bug to based on the MP4Reader was a mistake. Keep habing bitrot now.
Blocks: 1165585
Attached patch Remove locking to some members (obsolete) — Splinter Review
Another re-think. Remove the need for mIsFlushing and its associated lock. We now set a flag when we feed data to the decoder to indicate that we expect an output, and clear it after a flush. Anything received following a flush and prior a new request for data is as such unwanted and can safely be discarded. Re-organised the variable to make them in three categories: the ones related to Update(), the ones related to getting the state from the decoder, and the ones requiring the monitor.
Attachment #8606721 - Flags: review?(cpearce)
Attachment #8606606 - Attachment is obsolete: true
Attachment #8606606 - Flags: review?(cpearce)
Comment on attachment 8606721 [details] [diff] [review]
Remove locking to some members

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

::: dom/media/MediaFormatReader.h
@@ +275,3 @@
>      }
>      // Monitor that protects all non-threadsafe state; the primitives
>      // that follow.

These are used to feed into SizeOfVideoQueueInFrames(), which is accessed from the MDSM's task queue. That is only used for logging. So instead of making these synchronized by the monitor, you could just store the value reported by SizeOfVideoQueueInFrames() in an atomic int, and update whenever something changes, and then you don't need to lock when changing these.
Attachment #8606721 - Flags: review?(cpearce) → review+
Rebasing... Making it dependent of MP4Reader was a mistake
Attachment #8606146 - Attachment is obsolete: true
Attachment #8606147 - Attachment is obsolete: true
Attachment #8606148 - Attachment is obsolete: true
Rebase, carrying r+. Remove the need for locking for NumSampleInput/NumSampleOutput ; instead use an Atomic<size_t> mSizeOfQueue that is updated accordingly
Attachment #8606721 - Attachment is obsolete: true
Blocks: 1109433
Blocks: 1166779
Blocks: oggdemuxer
You need to log in before you can comment on or make changes to this bug.