Remove NotifyWaitingForResourcesStatusChanged() call from MediaOmxReader

RESOLVED FIXED in Firefox 41

Status

()

Core
Audio/Video
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: oliverthor, Assigned: sotaro)

Tracking

({crash})

unspecified
mozilla41
ARM
Gonk (Firefox OS)
crash
Points:
---

Firefox Tracking Flags

(firefox41 fixed, b2g-master affected)

Details

(Whiteboard: [3.0-Daily-Testing], crash signature)

Attachments

(6 attachments, 8 obsolete attachments)

700 bytes, patch
Details | Diff | Splinter Review
2.54 KB, patch
bwu
: review+
Details | Diff | Splinter Review
6.78 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
12.33 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
9.82 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
31.47 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
This crash report had been observed and resolved fixed one month ago, encountered in the Music app.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
See Also: → bug 1158307
Whiteboard: [3.0-Daily-Testing]
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: steps-wanted
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 2

3 years ago
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()
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
Component: Gaia::First Time Experience → Video/Audio
Product: Firefox OS → Core
(Assignee)

Updated

3 years ago
See Also: → bug 1146086
(Assignee)

Comment 3

3 years ago
(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
(Assignee)

Comment 4

3 years ago
Removing NotifyWaitingForResourcesStatusChanged() call seems correct way to fix the problem.
(Assignee)

Comment 5

3 years ago
Created attachment 8610242 [details] [diff] [review]
temporary patch - Enable MediaOmxReader usage for mp4
(Assignee)

Comment 6

3 years ago
Created attachment 8610244 [details] [diff] [review]
patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader
(Assignee)

Comment 7

3 years ago
Created attachment 8610246 [details] [diff] [review]
patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader

Update nits.
Attachment #8610244 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8610552 [details] [diff] [review]
patch - Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader

Fix RTSP poroblem.
Attachment #8610246 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8610555 [details] [diff] [review]
patch part 1 - Update OMXCodecProxy's callback similar to MediaCodecProxy::CodecResourceListener
Attachment #8610552 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8610560 [details] [diff] [review]
patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise
(Assignee)

Comment 11

3 years ago
Created attachment 8610562 [details] [diff] [review]
partch part 3 - Update OmxDecoder::AllocateMediaResources() to return MediaResourcePromise
(Assignee)

Comment 12

3 years ago
Created attachment 8610564 [details] [diff] [review]
patch part 4 - Update RtspOmxReader
(Assignee)

Updated

3 years ago
Summary: crash in mozilla::MediaTaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) → Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader
(Assignee)

Updated

3 years ago
Attachment #8610555 - Flags: review?(bwu)
(Assignee)

Updated

3 years ago
Attachment #8610560 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8610562 - Flags: review?(bwu)
Attachment #8610562 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8610564 - Flags: review?(bwu)
(Assignee)

Updated

3 years ago
Summary: Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader → Remove NotifyWaitingForResourcesStatusChanged() call from MediaOmxReader
(Assignee)

Updated

3 years ago
Blocks: 1168456
(Assignee)

Updated

3 years ago
No longer blocks: 1168456
(Assignee)

Comment 14

3 years ago
Created Bug 1168456 for MediaCodecReader.
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();|?
Attachment #8610560 - Flags: review?(bobbyholley) → review+
Attachment #8610562 - Flags: review?(bobbyholley) → review+
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.
Flags: needinfo?(ktucker)
(Assignee)

Comment 17

3 years ago
(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.
Flags: needinfo?(ktucker)
(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 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().
Attachment #8610555 - Flags: review?(bwu) → review+
Attachment #8610562 - Flags: review?(bwu) → review+
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
(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?
Flags: needinfo?(ettseng)
(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.
(Assignee)

Comment 23

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
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.
(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!
Flags: needinfo?(ettseng)
Attachment #8610564 - Flags: review?(bwu) → review+
(Assignee)

Comment 26

3 years ago
Created attachment 8611214 [details] [diff] [review]
patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

Apply the comments. Carry "r=bholley".
Attachment #8610560 - Attachment is obsolete: true
Attachment #8611214 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1168456
(Assignee)

Comment 28

3 years ago
Created attachment 8611458 [details] [diff] [review]
patch part 1 - Update OMXCodecProxy's callback similar to MediaCodecProxy::CodecResourceListener

Remove unused code.
Attachment #8610555 - Attachment is obsolete: true
Attachment #8611458 - Flags: review+
(Assignee)

Comment 29

3 years ago
Created attachment 8611459 [details] [diff] [review]
patch part 2 - Add MediaOmxReader::AsyncReadMetadata() to support MetadataPromise

Remove unused code.
Attachment #8611214 - Attachment is obsolete: true
Attachment #8611459 - Flags: review+
(Assignee)

Comment 30

3 years ago
Created attachment 8611461 [details] [diff] [review]
partch part 3 - Update OmxDecoder::AllocateMediaResources() to return MediaResourcePromise

Remove unused code.
Attachment #8610562 - Attachment is obsolete: true
Attachment #8611461 - Flags: review+
(Assignee)

Comment 31

3 years ago
Created attachment 8611463 [details] [diff] [review]
roll-up patch
(Assignee)

Comment 32

3 years ago
Created attachment 8611464 [details] [diff] [review]
patch_1167608_rollup
Attachment #8611463 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1210aa256080
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Clearing tag on resolved issue
Flags: needinfo?(ktucker)
Keywords: steps-wanted
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.