1215003 - MediaDecoderReader::AsyncReadMetadata should figure out which thread to dispatch the job

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[JW Wang [:jwwang] [:jw_wang]]

MediaDecoderReader::AsyncReadMetadata should figure out which thread to dispatch the job to hide the internal thread model of MediaDecoderReader from its client code. The client won't have to worry about on which thread this function should run.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1215003. Part 1 - Move MediaDecoderReader::ReadMetadata and overrides to private sections.

Bug 1215003. Part 1 - Move MediaDecoderReader::ReadMetadata and overrides to private sections.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1215003. Part 2 - rename AsyncReadMetadata and move it to the private section.

Bug 1215003. Part 2 - rename AsyncReadMetadata and move it to the private section.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1215003. Part 3 - fix AsyncReadMetadata() and its callers.

Bug 1215003. Part 3 - fix AsyncReadMetadata() and its callers.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Before I finish the reviews, something to consider:

:jya noticed your previous patches with the threadsafe public methods calling/dispatching private "Internal" specific-thread-only overrides, and made the (IMO) valid comment that this assumes that the method is actually *never* threadsafe. In other words, if one of the "Internal" override implementation is inherently threadsafe (e.g., it itself only does another dispatch), the new implementation will perform an unnecessary dispatch.
(jya, please correct me if I didn't represent your thinking correctly.)

Also, I think that it removes the ability for some savvy code to directly call the "Internal" method, and therefore the new implementation will always cost at least the OnTaskQueue() check even when it could be avoided.

A possible scheme to be more flexible and efficient could be something like:
In the base class:
> public:
>   // Thread-safe version, default calls/dispatch non-thread safe version.
>   virtual DoStuff() { if(OnTaskQueue){DoStuffOnTaskQueue();}else{Dispatch(OwnerThread(),DoStuffOnTaskQueue)} }
>   // Task-queue-only version, does the actual work (or '=0' for pure virtual).
>   virtual DoStuffOnTaskQueue() { assert(OnTaskQueue()); ... }

In one derived class where 'stuff' is not inherently treadsafe:
>   DoStuffOnTaskQueue() override { assert(OnTaskQueue()); ... }

In another derived class where 'stuff' is inherently threadsafe:
>   DoStuff() override { /* don't dispatch, just do stuff here */ ... }
>   // Need this in case it's called that way.
>   DoStuffOnTaskQueue() { DoStuff(); }

What do you think?
It could be something to revisit later on; but we should at least consider it before changing more interfaces to the "Internal" style.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8674650 [details]
MozReview Request: Bug 1215003. Part 1 - Move MediaDecoderReader::ReadMetadata and overrides to private sections.

https://reviewboard.mozilla.org/r/22273/#review19875

r+, whether or not you want to remove unnecessary 'virtual' keywords.

::: dom/media/android/AndroidMediaReader.h:89
(Diff revision 1)
> +  nsresult ReadMetadata(MediaInfo* aInfo, MetadataTags** aTags) override;

In recent bugs, you've kept the 'virtual' keyword when moving/modifying 'override' methods; I didn't complain then because I thought it was fine to stay consistent with the rest of the file.

Here you have removed the 'virtual' keyword (as per the recent coding style changes), but it means this line is now inconsistent with others in the same header.

I'm usually in favor of consistency over style guidelines. But it's not a big deal as we'll eventually probably remove all unnecessary 'virtual' keywords.
Please just be consistent with yourself though! If you've decided to remove all unnecesary 'virtual' keyworks, I'll expect you always do it from now on!

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8674651 [details]
MozReview Request: Bug 1215003. Part 2 - rename AsyncReadMetadata and move it to the private section.

https://reviewboard.mozilla.org/r/22275/#review19877

r+, preferably with the same 'virtual' keyword style as the first patch, but it's not critical.

::: dom/media/omx/MediaCodecReader.h:74
(Diff revision 1)
> +  virtual nsRefPtr<MediaDecoderReader::MetadataPromise>
> +  AsyncReadMetadataInternal() override;

Here you've chosen to keep 'virtual'!
Also, if you removed it, the whole line could probably fit in the 80-column limit. :-)

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8674652 [details]
MozReview Request: Bug 1215003. Part 3 - fix AsyncReadMetadata() and its callers.

https://reviewboard.mozilla.org/r/22277/#review19879

r+, assuming we keep forging ahead with the "Internal" scheme.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #7)
> I'm usually in favor of consistency over style guidelines. But it's not a
> big deal as we'll eventually probably remove all unnecessary 'virtual'
> keywords.
> Please just be consistent with yourself though! If you've decided to remove
> all unnecesary 'virtual' keyworks, I'll expect you always do it from now on!

Thanks for the review! I also put consistency before the style. I will restore the consistency of 'virtual' keywords before check-in and then file a new bug to remove all unnecessary 'virtual's so the style is consistent across comments.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

s/so the style is consistent across comments/so the style is consistent across commits/

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #6)
> What do you think?
> It could be something to revisit later on; but we should at least consider
> it before changing more interfaces to the "Internal" style.

Thanks for bringing up this design question. My goal is bug 1213764 which hides the internal thread model of MediaDecoderReader from its client code. So the client code will never be on the right thread to call DoStuffOnTaskQueue() safely.

On the other hand, what if a method is already thread-safe to avoid unnecessary dispatch? I think It depends on whether it is a virtual method or not. If no, we can always expose such a method to the client code so it can be called directly without being wrapped in a check-and-dispatch method. If yes, unless it is specified in the public API that all overrides must be thread-safe, it is hard to enforce all overrides must implement it in a thread-safe way. Therefore, check-and-dispatch is always necessary even though it might be less than optimal for some thread-safe overrides.

Thoughts?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf3ad7a207c
in case I break anything in restore 'virtual' keywords...

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #12)
> (In reply to Gerald Squelart [:gerald] from comment #6)
> > What do you think?
> > It could be something to revisit later on; but we should at least consider
> > it before changing more interfaces to the "Internal" style.
> 
> Thanks for bringing up this design question. My goal is bug 1213764 which
> hides the internal thread model of MediaDecoderReader from its client code.
> So the client code will never be on the right thread to call
> DoStuffOnTaskQueue() safely.
> 
> On the other hand, what if a method is already thread-safe to avoid
> unnecessary dispatch? I think It depends on whether it is a virtual method
> or not. If no, we can always expose such a method to the client code so it
> can be called directly without being wrapped in a check-and-dispatch method.
> If yes, unless it is specified in the public API that all overrides must be
> thread-safe, it is hard to enforce all overrides must implement it in a
> thread-safe way. Therefore, check-and-dispatch is always necessary even
> though it might be less than optimal for some thread-safe overrides.
> 
> Thoughts?

I understand what you're trying to achieve but I think the decision on how it should be implemented should be left up to the actual implementation.

E.g. The function is documented and designed to be called on any thread, (or in this case on the MDSM thread). So if the class overriding that particular function wants to requeue the task that's up to it.

You still achieve your goal that the MediaDecoderReader client (the MDSM) doesn't need to know about the MediaDecoderReader thread design.

If the code was originally thread-safe, then you don't need to bother with an unnecessary dispatch. The only thing that would allow to achieve bother would be to make both functions (Blah and BlahInternal) virtual: that becomes too much and hard to follow.

Same deal you had with ReleaseMediaResource, in this particular case, the dispatch was unnecessary as the MediaDataDecoder calls are themselves thread-safe.

If the developer is bad enough that he doesn't pay attention to thread safety in the first place, IMHO he will still produce bad code elsewhere, so what attempt to do so in the first place ? :)

Long-term wise, I'd like to only see MediaFormatReader anyway with MediaDecoderReader base class disappearing. You've made the task slightly more complicated as all the code in MediaDecoderReader base class will have to be shifted back to MediaFormatReader

My $0.02

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fccc21df6a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/837ab762d9c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4baa5f9c6f6d

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> I understand what you're trying to achieve but I think the decision on how
> it should be implemented should be left up to the actual implementation.
> 
> E.g. The function is documented and designed to be called on any thread, (or
> in this case on the MDSM thread). So if the class overriding that particular
> function wants to requeue the task that's up to it.
> 
> You still achieve your goal that the MediaDecoderReader client (the MDSM)
> doesn't need to know about the MediaDecoderReader thread design.
> 
> If the code was originally thread-safe, then you don't need to bother with
> an unnecessary dispatch. The only thing that would allow to achieve bother
> would be to make both functions (Blah and BlahInternal) virtual: that
> becomes too much and hard to follow.

In this case, we can just have:

RefPtr<MetadataPromise> AsyncReadMetadata()
{
  return AsyncReadMetadataInternal();
}

provided that all AsyncReadMetadataInternal() overrides must be thread-safe.

I would like to uphold the template method pattern in this case since it is easier to add pre/post conditions without modifying all overrides respectively.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> Same deal you had with ReleaseMediaResource, in this particular case, the
> dispatch was unnecessary as the MediaDataDecoder calls are themselves
> thread-safe.
Do you mean MediaDataDecoderCallback? Can you show me the related code?

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

This seems completely unnecessary to add an "Internal" suffix to the method makes implementing this clunky. Please back this out.

The MediaDecoderReader has only one user; the state machine, it is not an interface that's exposed to general users. The MediaDecoderReader interface is supposed to only be called on the decode task queue, that's why there are assertions throughout all the implementations ensuring that. That's why we went and have things like ProxyMediaCall to make this happen.

Please just use ProxyMediaCall (or whatever it got renamed to InvokeAsync?) and not add this extra layer.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

See bug 1213764. The goal is to apply the active object pattern to hide the internal thread model from its client code. There are several benefits from it:

1. By hiding the implementation detail, it is easier for the reader classes to evolve in the future.
2. It makes the client code (MDSM) more compact without worrying which thread the function should run.
   InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, &MediaDecoderReader::AsyncReadMetadata)->Then(...) v.s.
   mReader->AsyncReadMetadata()->Then(...)
3. We apply the template method pattern by separating Blah() and BlahInternal(). It is easier to apply pre/post conditions to Blah() (non-virtual) then to all overrides of BlahInternal() (a virtual method).
4. We can write gtests to test new reader features when mochitests are not possible yet (see bug 1194606). We want a more generic API for the reader so it can be used without creating MDSM.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

My main complaint is having to split Blah() function into Blah and BlahInternal(). Anything with XXInternal() is a terrible and messy name.

If you want to abstract the threading model out of the state machine, a less offensive approach would be to decorate the MediaDecoderReader with a proxy that calls the MediaDecoderReader on the right thread. I'd be OK with that.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

So here are some proposals:

1. Rename BlahInternal() to BlahImpl(). The name is shorter and less irritating.

2. A MediaDecoderReaderProxy to wrap around MediaDecoderReader as suggested by comment 20.

3. Move the check-thread-and-dispatch code from Blah() to BlahImpl(), remove Blah() and rename BlahImpl() back to Blah(). The downside is we have to repeat the check-thread-and-dispatch code in all Blah() overrides and we lose the benefit of template method pattern. But it is less a concern since jya said we will remove all MediaDecoderReader subclasses and leave MediaFormatReader. MediaFormatReader won't need virtual methods anymore. The polymorphism comes from sub-classes of MediaDataDecoder and MediaTrackDemuxer.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to JW Wang [:jwwang] from comment #21)
> So here are some proposals:
> 
> 1. Rename BlahInternal() to BlahImpl(). The name is shorter and less
> irritating.

Impl is still irritating.

> 2. A MediaDecoderReaderProxy to wrap around MediaDecoderReader as suggested
> by comment 20.

I support this, or backing out.


> 3. Move the check-thread-and-dispatch code from Blah() to BlahImpl(), remove
> Blah() and rename BlahImpl() back to Blah(). The downside is we have to
> repeat the check-thread-and-dispatch code in all Blah() overrides and we
> lose the benefit of template method pattern. But it is less a concern since
> jya said we will remove all MediaDecoderReader subclasses and leave
> MediaFormatReader. MediaFormatReader won't need virtual methods anymore. The
> polymorphism comes from sub-classes of MediaDataDecoder and
> MediaTrackDemuxer.

This seems like more work that's better done in the MSDM, i.e. worse than the status quo.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fccc21df6a5
https://hg.mozilla.org/mozilla-central/rev/837ab762d9c9
https://hg.mozilla.org/mozilla-central/rev/4baa5f9c6f6d

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fccc21df6a5
https://hg.mozilla.org/mozilla-central/rev/837ab762d9c9
https://hg.mozilla.org/mozilla-central/rev/4baa5f9c6f6d

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Per discussion of bug 1215003 comment 18 ~ bug 1215003 comment 22, I will backout related bugs and take the approach 2 to create MediaDecoderReaderProxy for handling thread details.

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

I will do it in bug 1216850.

Comment 27

[JW Wang [:jwwang] [:jw_wang]]

Just found out we already have https://hg.mozilla.org/mozilla-central/file/f029ccdee154/dom/media/MediaDecoderReader.h#l281.

If this pattern is unavoidable, will Blah() and DoBlah() give more pleasant names for you?

Comment 28

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #27)
> Just found out we already have
> https://hg.mozilla.org/mozilla-central/file/f029ccdee154/dom/media/
> MediaDecoderReader.h#l281.
> 
> If this pattern is unavoidable, will Blah() and DoBlah() give more pleasant
> names for you?

I think the main reason for this was that bholley implemented it in the evening at Whistler and he was rushing to get things done.

Also here, NotifyDataArrived / NotifyDataArrivedInternal perform two very different task in the end and most readers do not need a NotifyDataArrivedInternal, it's not a matter of simply dispatching to another thread.

I wasn't a fan of that to start with either :)

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to JW Wang [:jwwang] from comment #27)
> Just found out we already have
> https://hg.mozilla.org/mozilla-central/file/f029ccdee154/dom/media/
> MediaDecoderReader.h#l281.
> 
> If this pattern is unavoidable, will Blah() and DoBlah() give more pleasant
> names for you?

I'm not a fan of Blah() and XXXBlah(), because it means your abstraction of Blah() isn't pure; the fact that there are Blah and XXXBlah methods leaks into the subclass.

I'd prefer either a decorator to enforce the thread safety, or non virtual CallBlahOnTaskQueueX() on the base class with a virtual Blah() and implemented by subclasses, rather than subclasses implementing BlahInternal() methods.