Closed Bug 1203047 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Please see comment#1
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard) → needinfo?(jwwang)
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;
Flags: needinfo?(jwwang)
(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.
Flags: needinfo?(jyavenard)
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.
Assignee: nobody → jacheng
Attachment #8659079 - Flags: feedback?(jwwang)
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.
Attachment #8659079 - Flags: feedback?(jwwang) → feedback+
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.
Attachment #8659079 - Attachment is obsolete: true
Attachment #8659091 - Flags: review?(jyavenard)
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);
Attachment #8659091 - Flags: review?(jyavenard) → review+
Flags: needinfo?(jyavenard)
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.
Attachment #8659091 - Attachment is obsolete: true
Attachment #8659184 - Flags: review+
Keywords: checkin-needed
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/0be0207d4271
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.