Closed
Bug 1167608
Opened 10 years ago
Closed 10 years ago
Remove NotifyWaitingForResourcesStatusChanged() call from MediaOmxReader
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: onelson, Assigned: sotaro)
References
Details
(Keywords: crash, Whiteboard: [3.0-Daily-Testing])
Crash Data
Attachments
(6 files, 8 obsolete files)
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 |
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•10 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: → 1158307
Whiteboard: [3.0-Daily-Testing]
Updated•10 years ago
|
Updated•10 years ago
|
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 2•10 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•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Updated•10 years ago
|
Component: Gaia::First Time Experience → Video/Audio
Product: Firefox OS → Core
Assignee | ||
Comment 3•10 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•10 years ago
|
||
Removing NotifyWaitingForResourcesStatusChanged() call seems correct way to fix the problem.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Fix RTSP poroblem.
Attachment #8610246 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8610552 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Summary: crash in mozilla::MediaTaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) → Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader
Assignee | ||
Updated•10 years ago
|
Attachment #8610555 -
Flags: review?(bwu)
Assignee | ||
Updated•10 years ago
|
Attachment #8610560 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8610562 -
Flags: review?(bwu)
Attachment #8610562 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8610564 -
Flags: review?(bwu)
Assignee | ||
Updated•10 years ago
|
Summary: Remove NotifyWaitingForResourcesStatusChanged() from MediaOmxReader → Remove NotifyWaitingForResourcesStatusChanged() call from MediaOmxReader
Assignee | ||
Comment 14•10 years ago
|
||
Created Bug 1168456 for MediaCodecReader.
Comment 15•10 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]:
-----------------------------------------------------------------
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+
Updated•10 years ago
|
Attachment #8610562 -
Flags: review?(bobbyholley) → review+
Comment 16•10 years ago
|
||
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•10 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.
Updated•10 years ago
|
Flags: needinfo?(ktucker)
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8610562 -
Flags: review?(bwu) → review+
Comment 20•10 years ago
|
||
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•10 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.
>
> [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)
Comment 22•10 years ago
|
||
(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•10 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•10 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.
Comment 25•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8610564 -
Flags: review?(bwu) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Apply the comments. Carry "r=bholley".
Attachment #8610560 -
Attachment is obsolete: true
Attachment #8611214 -
Flags: review+
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Remove unused code.
Attachment #8610555 -
Attachment is obsolete: true
Attachment #8611458 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
Remove unused code.
Attachment #8611214 -
Attachment is obsolete: true
Attachment #8611459 -
Flags: review+
Assignee | ||
Comment 30•10 years ago
|
||
Remove unused code.
Attachment #8610562 -
Attachment is obsolete: true
Attachment #8611461 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8611463 -
Attachment is obsolete: true
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 35•9 years ago
|
||
Clearing tag on resolved issue
Flags: needinfo?(ktucker)
Keywords: steps-wanted
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•