1203047 - Make MediaDecoderReader know less about AudioData/VideoData by using MediaData instead

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

After discussion with JW,
Referring to bug 1194606 comment 8
We would like to make MDSM more abstract by replacing all interface from Audio/Video Data with MediaData.

It is inevitable to refactor the MediaDecoderReader interface

and replace the use of 
typedef MozPromise<nsRefPtr<AudioData>, NotDecodedReason, /* IsExclusive = */ true> AudioDataPromise;
  typedef MozPromise<nsRefPtr<VideoData>, NotDecodedReason, /* IsExclusive = */ true> VideoDataPromise;
by maybe
typedef MozPromise<nsRefPtr<MediaData>, NotDecodedReason, /* IsExclusive = */ true> AudioDataPromise;
typedef MozPromise<nsRefPtr<MediaData>, NotDecodedReason, /* IsExclusive = */ true> VideoDataPromise;

Since there are lots of classes derived from MediaDecoderReader, we will modify lots of files related to this interface.

Hi JYA,

Could you please give me some suggestion that if it is a right direction and any problems that may be noticed or encountered?

If so, I will start to work on this bug.

Thank you very much.

Comment 1

[James Cheng[:JamesCheng]]

Please see comment#1

Comment 2

[Jean-Yves Avenard [:jya]]

That sounds good to me.

Not sure why it would require to change lots of file. AudioData derive from MediaData.

One problem I can foresee is that nsRefPtr<C> doesn't allow conversion to nsRefPtr<T> when C inherit from T ; but already_AddRef does.

So what about changing AudioDataPromise to not be a typedef, but instead a class allow the implicit conversion.

JW will have a better idea I'm sure.

Comment 3

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

It depends on how we'd like to change the API of MediaDecoderReader to return demuxed but not decoded data.

1. Add the ability to Request{Audio,Video}Data so it can return both decoded or demuxed only data depending on how the reader is configured.
2. Add new functions such as Request{Audio,Video}RawData to return demuxed data.

If we pick option 2, we don't have to change AudioDataPromise. We will add a new typedef which is
typedef MozPromise<nsRefPtr<MediaRawData>, NotDecodedReason, /* IsExclusive = */ true> MediaRawDataPromise;

Comment 4

[James Cheng[:JamesCheng]]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> That sounds good to me.
> 
> Not sure why it would require to change lots of file. AudioData derive from
> MediaData.
> 
Sorry,

changing lots of file means once I modify the typedef into MediaData 
typedef MozPromise<nsRefPtr<MediaData>, NotDecodedReason, /* IsExclusive = */ true>

All the prototypes of the resolver function should be changed....

> One problem I can foresee is that nsRefPtr<C> doesn't allow conversion to
> nsRefPtr<T> when C inherit from T ; but already_AddRef does.
>
It seems it supports 
https://dxr.mozilla.org/mozilla-central/source/mfbt/nsRefPtr.h#109

Does it what you mention?

> So what about changing AudioDataPromise to not be a typedef, but instead a
> class allow the implicit conversion.
> 
> JW will have a better idea I'm sure.

Comment 5

[James Cheng[:JamesCheng]]

Attachment: v1-Make-MediaDecoderReader-know-less-about-.patch

Hi JW, 
As out discussion,
Please help to see if it is OK for the current design.

In summary,
I modified the resolver type
typedef MozPromise<nsRefPtr<AudioData>, NotDecodedReason, /* IsExclusive = */ true> AudioDataPromise;
typedef MozPromise<nsRefPtr<VideoData>, NotDecodedReason, /* IsExclusive = */ true> VideoDataPromise;

into MediaData and modified some related interface.


Thank you.

Comment 6

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

Comment on attachment 8659079 [details] [diff] [review]
v1-Make-MediaDecoderReader-know-less-about-.patch

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

::: dom/media/MediaDecoderStateMachine.h
@@ +777,5 @@
>      struct PromiseSampleType {
>        typedef typename PromiseType::ResolveValueType::element_type Type;
>      };
>  
> +    template<typename PromiseType, MediaData::Type SAMPLE_TYPE>

s/SAMPLE_TYPE/SampleType/ so the case is consistent.

Comment 7

[James Cheng[:JamesCheng]]

Attachment: Make-MediaDecoderReader-know-less-about-.patch

Hi JYA,

Could you please help for reviewing the patch?

The idea is to make the resolver function type become MediaData instead of A/VData.

Thank you very much.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8659091 [details] [diff] [review]
Make-MediaDecoderReader-know-less-about-.patch

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

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +198,1 @@
>  {

MOZ_ASSERT(aSample->mType == AUDIO_DATA);

@@ +374,1 @@
>  {

MOZ_ASSERT(aSample->mType == VIDEO_DATA);

Comment 9

[James Cheng[:JamesCheng]]

Attachment: Make-MediaDecoderReader-know-less-about-.patch

Thanks for reviewing.

Carry r+ from JYA.

Also modified VideoData* to MediaData* in AndroidMediaReader.cpp and MediaOmxReader.cpp to fix build error since those two instance did not instantiate on linux platform. 

https://dxr.mozilla.org/mozilla-central/source/dom/media/android/AndroidMediaReader.cpp#339

https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaOmxReader.cpp?from=MediaOmxReader.cpp&offset=0#545


Attach the treeherder result.

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

Thanks.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0be0207d4271

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0be0207d4271