Closed Bug 1157075 Opened 9 years ago Closed 9 years ago

Allow readers to use asyncronous ReadMetadata

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files, 1 obsolete file)

The media decoding architecture introduced in bug 1148292 is fundamentally asynchronous.

As such, making a synchronously compatible ReadMetadata() API isn't going to be possible without some hoops (in particular running the async read metadata on a new, independant task queue, while we wait for completion in ReadMetadata())

The MediaDecoderReader has a CallReadMetadata() method that provides an async-ish method.

We should be able to override that method.
The new MediaFormatDecoder is based on mostly asynchronous component. Therefore it needs to have a way to read metadata asynchronously without having to mess with new task queue etc.. Make CallMetadata() virtual and rename it to AsyncReadMetadata(). A MediaDecoderReader then needs to implement either ReadMetadata() or AsyncReadMetadata.
Fix an error in the comment that got me puzzled for a short while: IsWaitingMediaResources() isn't related to EME: otherwise this would prevent loadedmetadata to be fired for encrypted media
Attachment #8595926 - Flags: review?(bobbyholley)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Use new AsyncReadMetadata() to retrieve a reader's metadata. The MediaDecoderReader expects ReadMetadata to be called on the reader's task queue, but we were doing it on the TrackBuffer's task queue triggering the assert. I can't see any advantages in doing so on the's TrackBuffer's task queue.
Also, simplify aborts and shutdown: remove the decoder then, using the ability to simply disconnect a promise. I think MediaPromiseHolder.IsEmpty() should be renamed to Exists() for consistency, especially as it has RejectIfExists and ResolveIfExists
Attachment #8595933 - Flags: review?(bobbyholley)
Comment on attachment 8595933 [details] [diff] [review]
Part2. Use MediaPromise to read TrackBuffer metadata

Karl if you could also have a look as you've spent times already looking on when we need to remove the decoders from mDecoders etc.

During shutdown, all decoders from mDecoders are removed, that includes the one waiting to be initialized. So calling RemoveDecoder() on it served no purpose.

In regards to ReadMetadata previously running on the TrackBuffer's task queue was what caused bug 1156891. As it's the only time ReadMetadata could be called
Attachment #8595933 - Flags: feedback?(karlt)
Comment on attachment 8595926 [details] [diff] [review]
Part1. Rename and make AsyncReadMetadata virtual

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

::: dom/media/MediaDecoderReader.cpp
@@ +210,5 @@
>    nsRefPtr<MetadataHolder> metadata = new MetadataHolder();
>    nsresult rv = ReadMetadata(&metadata->mInfo, getter_Transfers(metadata->mTags));
>  
> +  // Reading metadata can cause us to discover that we need resources (a hardware
> +  // resource initialized but not yet ready for use)

Nit - add the trailing |.| back.

::: dom/media/MediaDecoderReader.h
@@ +170,5 @@
>    // The ReadMetadata API is unfortunately synchronous. We should fix that at
>    // some point, but for now we can make things a bit better by using a
>    // promise-y API on top of a synchronous call.
> +  // A MediaDecoderReader should implement either AsyncReadMetadata() API or
> +  // PreReadMetadata() / ReadMetadata().

I'd replace this whole comment with:

// The default implementation of AsyncReadMetadata is implemented in terms of
// synchronous PreReadMetadata() / ReadMetadata() calls. Implementations may also
// override AsyncReadMetadata to create a more proper async implementation.
Attachment #8595926 - Flags: review?(bobbyholley) → review+
Comment on attachment 8595933 [details] [diff] [review]
Part2. Use MediaPromise to read TrackBuffer metadata

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

The mechanics of this look ok, but I don't have a great sense of the high-level picture. Let's make sure it makes sense to karl.

::: dom/media/mediasource/TrackBuffer.cpp
@@ +565,4 @@
>    return true;
>  }
>  
> +class MetaDataRecipient {

Nit: { on next line.

Also add a comment indicating that this is basically just a closure over some extra values that the target methods need.

@@ -600,4 @@
>      return;
>    }
>  
> -  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());

Please replace this assert with a new one.

@@ +641,5 @@
>    {
>      ReentrantMonitorAutoExit mon(mParentDecoder->GetReentrantMonitor());
> +    mMetadataRequest.Begin(reader->AsyncReadMetadata()
> +                           ->RefableThen(mTaskQueue, __func__,
> +                                         new MetaDataRecipient(this, aDecoder, wasEnded),

Please init this on a separate line into an nsRefPtr. This is a bad pattern because it causes leaks in corner-cases where the callee never AddRefs the argument.

For situations where the callee _always_ needs to AddRef the argument, the solution is to take an already_AddRefed and have the caller pass do_AddRef (added in bug 1155059).

@@ +651,5 @@
> +void
> +TrackBuffer::OnMetadataRead(MetadataHolder* aMetadata,
> +                            SourceBufferDecoder* aDecoder,
> +                            bool aWasEnded)
> +{

Please assert the task queue that this is running in. We should do this for every method we can.

@@ +902,5 @@
>    DiscardCurrentDecoder();
> +
> +  if (mMetadataRequest.Exists() || !mInitializationPromise.IsEmpty()) {
> +    MOZ_ASSERT(current);
> +    RemoveDecoder(current);

I would end the scope of the |if| block here.
Attachment #8595933 - Flags: feedback?(karlt) → review?(karlt)
Comment on attachment 8595933 [details] [diff] [review]
Part2. Use MediaPromise to read TrackBuffer metadata

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

Er, meant to r+.
Attachment #8595933 - Flags: review?(bobbyholley) → review+
All of this is really temporary to make TrackBuffer works with the new MediaFormatReader...

All of this code will soon go.
Depends on: 1157203
(In reply to Bobby Holley (:bholley) from comment #5)
> Comment on attachment 8595933 [details] [diff] [review]
> Part2. Use MediaPromise to read TrackBuffer metadata
> 

> @@ +641,5 @@
> >    {
> >      ReentrantMonitorAutoExit mon(mParentDecoder->GetReentrantMonitor());
> > +    mMetadataRequest.Begin(reader->AsyncReadMetadata()
> > +                           ->RefableThen(mTaskQueue, __func__,
> > +                                         new MetaDataRecipient(this, aDecoder, wasEnded),
> 
> Please init this on a separate line into an nsRefPtr. This is a bad pattern
> because it causes leaks in corner-cases where the callee never AddRefs the
> argument.

you would typically pass "this" here.

The templated RefableThen expect ThisType* 

so if I do:
    nsRefPtr<MetaDataRecipient> recipient =
      new MetaDataRecipient(this, aDecoder, wasEnded);
    mMetadataRequest.Begin(reader->AsyncReadMetadata()
                           ->RefableThen(mTaskQueue, __func__,
                                         recipient,
                                         &MetaDataRecipient::OnMetadataRead,
                                         &MetaDataRecipient::OnMetadataNotRead));

that will give me:
 0:20.99 ../../../dist/include/MediaPromise.h:306:30: note: candidate template ignored: could not match 'ThisType *' against 'nsRefPtr<mozilla::MetaDataRecipient>'
 0:20.99   already_AddRefed<Consumer> RefableThen(AbstractThread* aResponseThread, const char* aCallSite, ThisType* aThisVal,
 0:20.99                              ^

passing an already_AddRefed instead by giving recipient.forget():
 0:16.94                            ->RefableThen(mTaskQueue, __func__,
 0:16.94                            ~~^~~~~~~~~~~
 0:16.94 ../../../dist/include/MediaPromise.h:306:30: note: candidate template ignored: could not match 'ThisType *' against 'already_AddRefed<mozilla::MetaDataRecipient>'
 0:16.94   already_AddRefed<Consumer> RefableThen(AbstractThread* aResponseThread, const char* aCallSite, ThisType* aThisVal,

RefableThen pass it to the templated ThenValue, which stores it inside the nsRefPtr.

Using the new do_AddRef wouldn't compile here either...

So I don't see how passing the pointer directly in this particular case is any different to using an intermediary nsRefPtr

Form-wise maybe, less ambiguity
pass .get()
(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Using the new do_AddRef wouldn't compile here either...

Yes, we would have to change the API to accept an already_AddRefed.

> So I don't see how passing the pointer directly in this particular case is
> any different to using an intermediary nsRefPtr

It is quite  different in the case that the callee never AddRefs the pointer: passing |new Foo()| will leak, because nobody ever gets the refcounting going. Using a temporary causes the object to get deallocated when the temporary goes out of scope, as one would expect.
Comment on attachment 8595933 [details] [diff] [review]
Part2. Use MediaPromise to read TrackBuffer metadata

Removing use of the initialization task queue sounds good to me, thanks.
There are some safety issues to address though.

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> During shutdown, all decoders from mDecoders are removed, that includes the
> one waiting to be initialized. So calling RemoveDecoder() on it served no
> purpose.

> TrackBuffer::InitializeDecoder(SourceBufferDecoder* aDecoder)

>   if (mShutdown) {
>     MSE_DEBUG("was shut down. Aborting initialization.");
>-    RemoveDecoder(aDecoder);
>     return;
>   }

Yes, it shouldn't be necessary to RemoveDecoder() when mShutdown because no
new decoders should be added by AppendData() after the
MediaSourceReader::Shutdown() has begun.

Now that InitializeDecoder() runs on the same task queue as
MediaSourceReader::Shutdown(), I don't think mShutdown is ever true here.
That could be asserted.

>   // We only reach this point once we know that we have a complete init segment.
>   // We don't want the demuxer to do a blocking read as no more data can be
>   // appended while this routine is running. Marking the SourceBufferResource
>   // as ended will cause any incomplete reads to abort.
>   // As this decoder hasn't been initialized yet, the resource isn't yet in use
>   // and so it is safe to do so.

I'm not clear about when incomplete reads may happen.
If we know we have a complete init segment, then are incomplete reads only for
frames after the init segment?
Will the MetadataPromise be resolved or rejected in that case?

>+  // Adding an empty buffer will reopen the SourceBufferResource
>+  if (aWasEnded) {

ITYM !aWasEnded

>-  if (NS_FAILED(rv) || (!mi.HasVideo() && !mi.HasAudio())) {
>-    // XXX: Need to signal error back to owning SourceBuffer.
>-    MSE_DEBUG("Reader %p failed to initialize rv=%x audio=%d video=%d",
>-              reader, rv, mi.HasAudio(), mi.HasVideo());
>-    RemoveDecoder(aDecoder);
>-    mInitializationPromise.RejectIfExists(NS_ERROR_FAILURE, __func__);
>-    return;
>-  }
>+  MediaInfo mi = aMetadata->mInfo;

Please assert mi.HasVideo() || mi.HasAudio() or handle the other case.

>+  if (mMetadataRequest.Exists() || !mInitializationPromise.IsEmpty()) {
>+    MOZ_ASSERT(current);
>+    RemoveDecoder(current);
>+    mMetadataRequest.DisconnectIfExists();

With the decoder removed on the main thread here, is there something that
keeps the decoder alive while its raw pointer is passed to
the CompleteInitializeDecoder() runnable?

Depending on comparisons of pointers to deleted objects would be dangerous
because new objects of the same type are likely to have the same pointer value
as deleted objects

>+  friend class MetaDataRecipient;

Metadata is one word.

>+  // Track our request for metadata from the reader.
>+  MediaPromiseConsumerHolder<MediaDecoderReader::MetadataPromise> mMetadataRequest;

This is Disconnect()ed on the main thread in the parent decoder monitor, but
Begin()ed on the decode thread outside of the monitor.

I don't think that can be safe.
It would at least allow the request to be Begin()ed after it has
been Disconnect()ed.

If there is a reason why it is safe, then please document here.
Attachment #8595933 - Flags: review?(karlt)
Attachment #8595933 - Flags: review-
Attachment #8595933 - Flags: feedback+
(In reply to Karl Tomlinson (:karlt) from comment #11)
> Now that InitializeDecoder() runs on the same task queue as
> MediaSourceReader::Shutdown(), I don't think mShutdown is ever true here.
> That could be asserted.

I didn't want to go down to much of that change, simply because I feel this should be done in another patch. And that entire code will disappear very soon.

This patch is only to allow the current TrackBuffer to work with the new demuxer's API
> 
> I'm not clear about when incomplete reads may happen.
> If we know we have a complete init segment, then are incomplete reads only
> for
> frames after the init segment?
> Will the MetadataPromise be resolved or rejected in that case?

we don't implement aborting partial media segment as per spec. The only thing we handle is aborting a partial init segment and continuing on. Which is something required for passing some w3c tests.

It's only that particular case we handle.

The patch doesn't change the existing behaviour, which is what I feel you're requiring now.

> >+  MediaInfo mi = aMetadata->mInfo;
> 
> Please assert mi.HasVideo() || mi.HasAudio() or handle the other case.

Why? the MetaDataPromise is only marked as resolved if we have either audio, video or both. So I fail to see the need for asserts. 


> 
> >+  if (mMetadataRequest.Exists() || !mInitializationPromise.IsEmpty()) {
> >+    MOZ_ASSERT(current);
> >+    RemoveDecoder(current);
> >+    mMetadataRequest.DisconnectIfExists();
> 
> With the decoder removed on the main thread here, is there something that
> keeps the decoder alive while its raw pointer is passed to
> the CompleteInitializeDecoder() runnable?

we hold the lock here. And mCurrentDecoder will now be nullptr, so as soon as we release the lock, if CompleteInitializeDecoder were to run, it would detect it there.


> 
> Depending on comparisons of pointers to deleted objects would be dangerous
> because new objects of the same type are likely to have the same pointer
> value
> as deleted objects

no, because a new decoder can never be created until we're finished, as TrackBuffer::AppendData can't be called in the mean time (not until updateend has been issued)

> 
> This is Disconnect()ed on the main thread in the parent decoder monitor, but
> Begin()ed on the decode thread outside of the monitor.

I thought about this, but decided it didn't matter as this operation is atomic enough.

There's no way around the problem I'm afraid. Because we have a fundamentally racey conditon between the.
(In reply to Karl Tomlinson (:karlt) from comment #11)

> >   // We only reach this point once we know that we have a complete init segment.
> >   // We don't want the demuxer to do a blocking read as no more data can be
> >   // appended while this routine is running. Marking the SourceBufferResource
> >   // as ended will cause any incomplete reads to abort.
> >   // As this decoder hasn't been initialized yet, the resource isn't yet in use
> >   // and so it is safe to do so.
> 
> I'm not clear about when incomplete reads may happen.
> If we know we have a complete init segment, then are incomplete reads only
> for
> frames after the init segment?

We discussed this over vidyo but for the record.

The issue was with the webm demuxer only. The webm demuxer attempts to read the entire stream you're passing and does not stop after reading the metadata. So if you had a partial media segment following the init segment it would have entered a blocking read waiting for the remaining of the cluster.

Yes ReadMetadata would have failed.

However, that problem was later fixed in bug 1125776, we were limit the webm read to the init segment content.

So that entire section is now unecessary.
Reworked so modifying the MetadataPromise is done while the lock is held. Ensure the promise is executed on the reader's taskqueue. We could get rid entirely of the TaskBuffer task queue, but that's for another day.
Attachment #8596974 - Flags: review?(karlt)
Attachment #8595933 - Attachment is obsolete: true
Comment on attachment 8596974 [details] [diff] [review]
Part2. Use MediaPromise to read TrackBuffer metadata

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> > Please assert mi.HasVideo() || mi.HasAudio() or handle the other case.
> 
> Why? the MetaDataPromise is only marked as resolved if we have either audio,
> video or both. So I fail to see the need for asserts. 

OK.  I didn't know that.  I usually assume that behavior not documented in the
interface declaration is not guaranteed, and so I like to assert about such
assumptions.

> > >+  if (mMetadataRequest.Exists() || !mInitializationPromise.IsEmpty()) {
> > >+    MOZ_ASSERT(current);
> > >+    RemoveDecoder(current);
> > >+    mMetadataRequest.DisconnectIfExists();
> > 
> > With the decoder removed on the main thread here, is there something that
> > keeps the decoder alive while its raw pointer is passed to
> > the CompleteInitializeDecoder() runnable?
> 
> we hold the lock here. And mCurrentDecoder will now be nullptr, so as soon
> as we release the lock, if CompleteInitializeDecoder were to run, it would
> detect it there.

I didn't know that NS_NewRunnableMethodWithArg held a reference to nsISupports
args, which addresses my concern here.

> > Depending on comparisons of pointers to deleted objects would be dangerous
> > because new objects of the same type are likely to have the same pointer
> > value
> > as deleted objects
> 
> no, because a new decoder can never be created until we're finished, as
> TrackBuffer::AppendData can't be called in the mean time (not until
> updateend has been issued)

A new decoder can be created after abort() is called and new data is appended.

> > This is Disconnect()ed on the main thread in the parent decoder monitor, but
> > Begin()ed on the decode thread outside of the monitor.
> 
> I thought about this, but decided it didn't matter as this operation is
> atomic enough.
> 
> There's no way around the problem I'm afraid. Because we have a
> fundamentally racey conditon between the.

The issue I was concerned about was addressed in InitializeDecoder, thanks, by
entering the monitor and checking for an abort() before adding the callbacks
to the promise.

>+  // Track our request for metadata from the reader.
>+  MediaPromiseConsumerHolder<MediaDecoderReader::MetadataPromise> mMetadataRequest;

Please document here that multi-thread access is protected by the parent
decoder monitor.
Attachment #8596974 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/5bb68945a86e
https://hg.mozilla.org/mozilla-central/rev/9d6fc27d1c77
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.