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

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.

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;

(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.

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 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.

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 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);

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.