Closed Bug 1036849 Opened 6 years ago Closed 5 years ago

Video part in MP4 DASH on B2G does not work

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
tracking-b2g backlog

People

(Reporter: bwu, Assigned: ajones)

References

Details

Attachments

(5 files, 8 obsolete files)

8.44 KB, patch
Details | Diff | Splinter Review
16.47 KB, patch
Details | Diff | Splinter Review
18.40 KB, patch
bwu
: review+
Details | Diff | Splinter Review
13.07 KB, patch
jya
: review+
Details | Diff | Splinter Review
20.35 KB, application/pdf
Details
Failed to play http://dashif.org/reference/players/javascript/1.0.0/baseline.html with Flame. 

Environment:
OS version: 2.1.0.0-prerelease (33.0a1) with the patches on bug 941302 and enable the pref (media.mediasource.enabled and media.fragmented-mp4.enabled).  

Symptom:
Show error message, "Codec (video/mp4;codecs="avc1.4d400D" is not supported)"
Blocks: MSE-FxOS
blocking-b2g: --- → 2.1?
The purpose of this bug is to make sure MP4 DASH can work on B2G besides firefox browser.
This is part of 2.1 feature work.
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1
Assignee: nobody → bwu
Target Milestone: --- → 2.1 S2 (15aug)
Based on the discussion with the partner, agreed to remove the feature-b2g flag on this.
feature-b2g: 2.1 → ---
Update the current status. 
After pulling today's latest codes and enabling media.fragmented-mp4.gonk.enabled and media.mediasource.enabled, the playback on http://people.mozilla.org/~cpearce/simple-dash/ cannot be started. It just shows a circling icon.
It looks like it is waiting something in upper layer and MP4Reader is not created yet.
ajones, 
It looks like it got stuck in the upper layer, probably in MSE. 
Could your team help further check?
Thanks!
Assignee: bwu → nobody
Flags: needinfo?(ajones)
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Comment on attachment 8479802 [details] [diff] [review]
WIP - YouTube on Flame

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

::: content/media/MediaDecoderReader.h
@@ +113,5 @@
>    // Note: DecodeVideoFrame, DecodeAudioData, ReadMetadata and Seek should
>    // activate the decoder if necessary. The state machine only needs to know
>    // when to call SetIdle().
>    virtual void SetIdle() { }
> +  virtual void SetActive() { }

Do you need an explicit SetActive(), or can the sub-reader assume that it should move to active state when Requset*Data() is called?

You'd need to be careful to ResetDecode() when the sub-reader is made idle, so that no pending tasks in the queue cause it to awaken.

We previously had discrete SetActive() and SetIdle() functions, and found managing the two states was unreliable. It may be unavoidable in this case though.

If you switch to using ReleaseMediaResources(), then you won't need this, you'll instead need to ensure that the reader re-does its ReadMetadata() path.

::: content/media/fmp4/MP4Reader.cpp
@@ +576,5 @@
>  
>  void
> +MP4Reader::SetIdle()
> +{
> +  if (mVideo.mActive && mVideo.mDecoder) {

SetIdle() is supposed to move the reader into a low power state. We call SetIdle() when playback is paused once the MediaQueue are full, so this will presumably slow down re-starting playback (I presume).

ReleaseMediaResources() the function that is supposed to release the decoder.

You should probably be using ReleaseMediaResources() here instead of SetIdle().
Attached patch WIP - YouTube on Flame (obsolete) — Splinter Review
YouTube videos play right through most of the time but there is no sound
Attachment #8479802 - Attachment is obsolete: true
Flags: needinfo?(ajones)
Attached patch Share a single MediaSourceReader (obsolete) — Splinter Review
The config part is a bit hacky but I think we can follow up with a proper fix that will work on desktop. In the meanwhile it's #ifdef'ed out in MP4Reader::SetSharedDecoderManager()
Attachment #8481740 - Flags: review?(kinetik)
Attachment #8480372 - Attachment is obsolete: true
Blake - This fixes the MSE specific issue of codec sharing. There is still an issue when you navigate to another video.
What do you mean by navigating to another video?
The first video plays through. The next one doesn't. BTW you will need to put MP4 in the MSE whitelist instead of using ignore_codecs. I'll post a patch for that.
Attachment #8481740 - Flags: review?(kinetik) → review?(edwin)
Summary: MP4 DASH on B2G cannot work → MP4 DASH on B2G does not work
Comment on attachment 8481740 [details] [diff] [review]
Share a single MediaSourceReader

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

Minor style stuff, because I can't find anything better to complain about.

::: content/media/fmp4/SharedDecoderManager.cpp
@@ +12,5 @@
> +{
> +public:
> +  SharedDecoderCallback(SharedDecoderManager* aManager) : mManager(aManager) {}
> +
> +  virtual void Output(MediaData* aData)

MOZ_OVERRIDE here as well, just to be anal.

::: content/media/fmp4/SharedDecoderManager.h
@@ +61,5 @@
> +  virtual nsresult Init();
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample);
> +  virtual nsresult Flush();
> +  virtual nsresult Drain();
> +  virtual nsresult Shutdown();

Minor style nit: MOZ_OVERRIDE.
Attachment #8481740 - Flags: review?(edwin) → review+
Summary: MP4 DASH on B2G does not work → Video part in MP4 DASH on B2G does not work
This is part of dependencies for support MSE on B2G
feature-b2g: --- → 2.1
Attachment #8481740 - Attachment is obsolete: true
And back out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5b992c20ee16 because gcc didn't like the includes.
removed feature-b2g: 2.1 as discussed via mail.
blocking-b2g: --- → backlog
feature-b2g: 2.1 → ---
Comment on attachment 8482041 [details] [diff] [review]
Fix dormancy call backs and shutdown path

Thanks for your suggestion! 
I am going to look into MSE codes more to have an overall understanding first and come out a solution.
Attachment #8482041 - Flags: feedback?(bwu)
Since on B2G currently there is only one instance of HW video decoder and IIUC currently MSE creates a new decoder for each chunk, we need to find a way to share this one decoder with MSE architecture. Attachment 8482225 [details] [diff] looks like a promising solution but it still cannot work well. I will follow up to check if something is missed or there are some problems in decoder side.
Assignee: ajones → bwu
jya may also want to use this approach here for the Apple hardware decoder, which has a limit of 2 active instances?
The limit depends on the GPU, and is only a problem on 10.6 machines where VDA is the only decoder available.

In 10.7 and later, we first use VDA (which has the limit) and then use VideoToolbox which doesn't (it's software only until 10.9).

So not sure it's worth bothering about it on mac.
More info about B2G's HW decoder, to init decoder and get the first decoded frame may take around 200ms, so current MSE design (create a new decoder for a new chunk) may not be suitable for B2G. I may create a "decoder manager" to not create a new decoder and shutdown the decoder for each chunk.
After some investigations, it might be better to have current MSE design refactored or changed to not create a new decoder for each new chunk. To have a wrapper or manager to make MSE able to create a new decoder for a new chunk but actually use one HW decoder to handle that might not be a good way since the changes is not minor and it needs to be removed after MSE design is changed. If there is no schedule pressure, we should not do that.    

Anthony, 
Do you have schedule or plan to change MSE design? Or any ideas?
Thanks!
Flags: needinfo?(ajones)
(In reply to Blake Wu [:bwu][:blakewu] from comment #27)
> Do you have schedule or plan to change MSE design? Or any ideas?

At the moment MediaSourceReader makes a MediaDecoderReader out of several MediaDecoderReaders. The ideal solution would be to modify it so that it creates a demuxer out of several demuxers instead. This is a significant change.
Flags: needinfo?(ajones)
Add the missed ShareDecoderManager files.
Assignee: bwu → ajones
Comment on attachment 8521902 [details] [diff] [review]
Create ShareDecoderManager and SharedDecoderProxy

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

Thanks for this patch! It could work. It looks like it is intended to not shutdown decoder when a chunk finishes decoding. 
I tried this patch the other day and its playback is not so smooth (probably related to bug 1043274). And sometimes at first time playback will not be started.

::: dom/media/fmp4/MP4Reader.cpp
@@ +895,5 @@
> +    mSharedDecoderManager->SetIdle(mVideo.mDecoder);
> +    NotifyResourcesStatusChanged();
> +  }
> +}
> +

SetIdle may not be required to set decoder idle to make it *drain* and *flush* as in SharedDecoderManager for a new chunk. For the new coming chunk data, it can be directly input to decoder. I am not sure if decoder will take some time to do flush which may not be able to decode the next new data in time. But If we don't drain and flush for each chunk, here comes a question: when to drain and flush when entire playback is going to end? I am still trying to find a good way.

::: dom/media/fmp4/SharedDecoderManager.cpp
@@ +71,5 @@
> +  layers::LayersBackend aLayersBackend, layers::ImageContainer* aImageContainer,
> +  MediaTaskQueue* aVideoTaskQueue, MediaDataDecoderCallback* aCallback)
> +{
> +  if (!mDecoder) {
> +    mConfig = aConfig;

mConfig may not be required. I don't see where it will be used.

@@ +84,5 @@
> +  }
> +
> +  nsRefPtr<SharedDecoderProxy> proxy(
> +    new SharedDecoderProxy(this, aCallback, aConfig));
> +  return proxy.forget();

We may not need create a new proxy for a new MP4Reader. One proxy should be sufficient for the entire playback and there is no need to have mActiveProxy.

@@ +89,5 @@
> +}
> +
> +void
> +SharedDecoderManager::Select(SharedDecoderProxy* aProxy)
> +{

If one proxy is sufficient, no need to select.

@@ +103,5 @@
> +
> +void
> +SharedDecoderManager::SetIdle(MediaDataDecoder* aProxy)
> +{
> +  if (aProxy && mActiveProxy == aProxy) {

As commented as above.

@@ +131,5 @@
> +SharedDecoderProxy::SharedDecoderProxy(
> +  SharedDecoderManager* aManager, MediaDataDecoderCallback* aCallback,
> +  const mp4_demuxer::VideoDecoderConfig& aConfig)
> +  : mManager(aManager), mCallback(aCallback), mConfig(aConfig)
> +{

This mConfig may not be required either!?
Attachment #8521902 - Flags: review?(bwu)
Attachment #8482041 - Attachment is obsolete: true
Attachment #8482182 - Attachment is obsolete: true
Attachment #8482225 - Attachment is obsolete: true
Attached patch Clean up AnnexB handling (obsolete) — Splinter Review
Attachment #8522646 - Flags: review?(jyavenard)
Attachment #8521902 - Attachment is obsolete: true
Target Milestone: 2.1 S2 (15aug) → ---
Comment on attachment 8522646 [details] [diff] [review]
Clean up AnnexB handling

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

I would leave FFmpeg untouched, as it can handle both AVCC and AnnexB ; no need to always convert everything to AnnexB there. Prevent potential regression with older version of FFmpeg/libav

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +395,1 @@
>      // Forward sample data to the decoder.

mConfig is now unused, and has to be removed

::: dom/media/fmp4/wmf/WMFVideoMFTManager.cpp
@@ +146,5 @@
>  HRESULT
>  WMFVideoMFTManager::Input(mp4_demuxer::MP4Sample* aSample)
>  {
>    // We must prepare samples in AVC Annex B.
> +  mp4_demuxer::AnnexB::ConvertSample(aSample);

mConfig is now unused here too

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +158,1 @@
>  

I'm sure there are some ways to make that type even more complicated :)
Attachment #8522646 - Flags: review?(jyavenard)
Attachment #8522646 - Attachment is obsolete: true
Attachment #8523580 - Flags: review?(jyavenard) → review+
Comment on attachment 8522693 [details] [diff] [review]
Create ShareDecoderManager and SharedDecoderProxy

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

Look good to me! 
Still wonder if setIdle and flush will take decoder some time and cannot decode the next new data in time :)
Anyway, we can fire another a bug to fix it if it is a problem.
Attachment #8522693 - Flags: review?(bwu) → review+
https://hg.mozilla.org/mozilla-central/rev/6fcc8ba98dbb
https://hg.mozilla.org/mozilla-central/rev/80d8ee785026
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attached file MSE block diagram
Upload what I understand about MSE for this bug.
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.