1167608 - Remove NotifyWaitingForResourcesStatusChanged() call from MediaOmxReader

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Oliver Nelson [:oliverthor]]

This bug was filed from the Socorro interface and is 
report bp-8294362d-5718-4e09-8190-c42422150522.
=============================================================

Encountered a crash report after flashing the nightly from 05/21 [20150521010203] and performing the tutorial in the FTU. After tapping through the FTE, the user  will reach the tutorial last that enables them to skip through animated images. After skipping through 2 screens, the screen flashed white and dropped to homescreen.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150521010203
Gaia: 5a7f87b1505ba89b586372cbbbe9507d1016c40c
Gecko: b9424d63fe35
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Comment 1

[Oliver Nelson [:oliverthor]]

This crash report had been observed and resolved fixed one month ago, encountered in the Music app.

Comment 2

[Sotaro Ikeda [:sotaro]]

The crash seems to be happened by posting a new task to already shutting down task queue. But from the crash log, the crash seems to be caused by a different code than bug 1158307.

The following function is called when video codec is reserved.
 MediaDecoder::NotifyWaitingForResourcesStatusChanged()

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Oliver Nelson [:oliverthor] from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-8294362d-5718-4e09-8190-c42422150522.

Since Bug 1060900 fix, default master flame-kk should use MP4Decoder instead of OmxDecoder. But the crash says that the device used OmxDecoder to decode mp4. It seems to say that pref of "media.fragmented-mp4.exposed" is overridden on the crashed device.
  https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#288

Comment 4

[Sotaro Ikeda [:sotaro]]

Removing NotifyWaitingForResourcesStatusChanged() call seems correct way to fix the problem.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Enable MediaOmxReader usage for mp4

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader

Update nits.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader

Fix RTSP poroblem.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Update OMXCodecProxy's callback similar to MediaCodecProxy::CodecResourceListener

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: partch part 3 - Update OmxDecoder::AllocateMediaResources() to return MediaResourcePromise

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Update RtspOmxReader

Comment 13

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e25acb7913

Comment 14

[Sotaro Ikeda [:sotaro]]

Created Bug 1168456 for MediaCodecReader.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8610560 [details] [diff] [review]
patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

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

This looks awesome!

::: dom/media/omx/MediaOmxReader.cpp
@@ -236,5 @@
>    }
>    return NS_OK;
>  }
>  
> -void MediaOmxReader::PreReadMetadata()

We can get rid of PreReadMetadata now, right? Can you add a followup patch (or file a followup bug) for doing that?

@@ +254,5 @@
> +    mOmxDecoder->AllocateMediaResources()
> +      ->RefableThen(GetTaskQueue(), __func__,
> +                    this,
> +                    &MediaOmxReader::OnResourceAllocated,
> +                    &MediaOmxReader::OnResourceAllocateFailed));

I'd call this OnResourceNotAllocated or OnResourceAllocationFailed.

These methods are quite short, so you might also consider inlining them as lambdas:

nsRefPtr<MediaOmxReader> self = this;
mMediaResourceRequest.Begin(mOmxDecoder->AllocateMediaResources()
  ->RefableThen(GetTaskQueue(), __func__,
    [self] () -> void { self->mMediaResourceRequest.Complete(); self->HandleResourceAllocated(); },
    [self] () -> void { self->mMediaResourceRequest.Complete(); self->mMetadataPromise.Reject(...); }
);

either way is fine with me.

Once bug 1168008 lands, you could even get rid of mMediaResourceRequest, and just have AsyncReadMetadata do:

return mOmxDecoder->AllocateMediaResources()->Then(&MediaOmxReader::HandleResourceAllocated, &MediaOmxReader::Pass);

But if you want to land before that, that's totally fine too. :-)

::: dom/media/omx/MediaOmxReader.h
@@ +100,5 @@
>    virtual nsresult ReadMetadata(MediaInfo* aInfo,
> +                                MetadataTags** aTags) override
> +  {
> +    // Unused as we provide AsyncReadMetadataAPI.
> +    // However we must implement it as it's pure virtual.

Instead of doing this, can you just add a default implementation on MediaDecoderReader that does |MOZ_CRASH();|?

Comment 16

[Pi Wei Cheng [:piwei] (inactive)]

I was unable to reproduce the bug after one hour of trying on the reported build. Repeatedly flashed the device in order to enter FTE and skipping through the FTE tutorial did not encounter any crash.

Device: Flame (full flashed 319MB kk)
BuildID: 20150521010203
Gaia: 5a7f87b1505ba89b586372cbbbe9507d1016c40c
Gecko: b9424d63fe35
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Leaving steps-wanted for others to attempt.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Pi Wei Cheng [:piwei] from comment #16)
> I was unable to reproduce the bug after one hour of trying on the reported
> build. Repeatedly flashed the device in order to enter FTE and skipping
> through the FTE tutorial did not encounter any crash.
> 

A thing in comment 3 might be affected to it.

Comment 18

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> (In reply to Pi Wei Cheng [:piwei] from comment #16)
> > I was unable to reproduce the bug after one hour of trying on the reported
> > build. Repeatedly flashed the device in order to enter FTE and skipping
> > through the FTE tutorial did not encounter any crash.
> > 
> 
> A thing in comment 3 might be affected to it.
Yeah. For mp4 files (FTU videos), now it will be played via MediaFormatReader[1], instead of MediaOmxReader. 

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp?from=MP4Decoder.cpp&case=true#35

Comment 19

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8610555 [details] [diff] [review]
patch part 1 - Update OMXCodecProxy's callback similar to MediaCodecProxy::CodecResourceListener

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

Looks good to me!

::: dom/media/omx/OMXCodecProxy.h
@@ +85,1 @@
>      void notifyStatusChangedLocked();

notifyStatusChangedLocked could be removed since it is replaced by your notifyResourceReserved() and notifyResourceCanceled().

Comment 20

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8610564 [details] [diff] [review]
patch part 4 - Update RtspOmxReader

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

::: dom/media/omx/RtspOmxReader.cpp
@@ -90,5 @@
>  {
>    // Send a PLAY command to the RTSP server before reading metadata.
>    // Because we might need some decoded samples to ensure we have configuration.
>    mRtspResource->DisablePlayoutDelay();
> -  EnsureActive();

This EnsureActive() should be necessary since it is required to start RTSP streaming.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/RtspOmxReader.cpp?from=RtspOmxReader.cpp#79

Comment 21

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> Comment on attachment 8610564 [details] [diff] [review]
> patch part 4 - Update RtspOmxReader
> 
> Review of attachment 8610564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/omx/RtspOmxReader.cpp
> @@ -90,5 @@
> >  {
> >    // Send a PLAY command to the RTSP server before reading metadata.
> >    // Because we might need some decoded samples to ensure we have configuration.
> >    mRtspResource->DisablePlayoutDelay();
> > -  EnsureActive();
> 
> This EnsureActive() should be necessary since it is required to start RTSP
> streaming.
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/
> RtspOmxReader.cpp?from=RtspOmxReader.cpp#79

Ethan, 
Is it required to call controller->Play() before reading metadata?

Comment 22

[Ethan Tseng [:ethan]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #21)
> Ethan, 
> Is it required to call controller->Play() before reading metadata?

I remembered the answer is yes. But I cannot recall the exact reason for now.
I'll check the code and let you know it later.

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> Comment on attachment 8610564 [details] [diff] [review]
> patch part 4 - Update RtspOmxReader
> 
> Review of attachment 8610564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/omx/RtspOmxReader.cpp
> @@ -90,5 @@
> >  {
> >    // Send a PLAY command to the RTSP server before reading metadata.
> >    // Because we might need some decoded samples to ensure we have configuration.
> >    mRtspResource->DisablePlayoutDelay();
> > -  EnsureActive();
> 
> This EnsureActive() should be necessary since it is required to start RTSP
> streaming.

EnsureActive() is called on top of MediaOmxReader::AsyncReadMetadata(). We do not need to call twice.

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8610560 [details] [diff] [review]
patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

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

::: dom/media/omx/MediaOmxReader.cpp
@@ -236,5 @@
>    }
>    return NS_OK;
>  }
>  
> -void MediaOmxReader::PreReadMetadata()

PreReadMetadata() is also implemented in MediaCodecReader. In bug 1168456, I am going to remove PreReadMetadata().

@@ +254,5 @@
> +    mOmxDecoder->AllocateMediaResources()
> +      ->RefableThen(GetTaskQueue(), __func__,
> +                    this,
> +                    &MediaOmxReader::OnResourceAllocated,
> +                    &MediaOmxReader::OnResourceAllocateFailed));

Thanks for the detailed comment! I'll update in a next patch.

::: dom/media/omx/MediaOmxReader.h
@@ +100,5 @@
>    virtual nsresult ReadMetadata(MediaInfo* aInfo,
> +                                MetadataTags** aTags) override
> +  {
> +    // Unused as we provide AsyncReadMetadataAPI.
> +    // However we must implement it as it's pure virtual.

Yes, it seems better. I just copied the code from MediaFormatReader.

Comment 25

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> > Comment on attachment 8610564 [details] [diff] [review]
> > patch part 4 - Update RtspOmxReader
> > 
> > Review of attachment 8610564 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/omx/RtspOmxReader.cpp
> > @@ -90,5 @@
> > >  {
> > >    // Send a PLAY command to the RTSP server before reading metadata.
> > >    // Because we might need some decoded samples to ensure we have configuration.
> > >    mRtspResource->DisablePlayoutDelay();
> > > -  EnsureActive();
> > 
> > This EnsureActive() should be necessary since it is required to start RTSP
> > streaming.
> 
> EnsureActive() is called on top of MediaOmxReader::AsyncReadMetadata(). We
> do not need to call twice.
Yeah. You are right!

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

Apply the comments. Carry "r=bholley".

Comment 27

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=510c0f5e6ec4

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Update OMXCodecProxy's callback similar to MediaCodecProxy::CodecResourceListener

Remove unused code.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

Remove unused code.

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: partch part 3 - Update OmxDecoder::AllocateMediaResources() to return MediaResourcePromise

Remove unused code.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: roll-up patch

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch_1167608_rollup

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1210aa256080

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1210aa256080

Comment 35

[Pi Wei Cheng [:piwei] (inactive)]

Clearing tag on resolved issue