Closed
Bug 1214506
Opened 9 years ago
Closed 9 years ago
crash in mozilla::AudioChunk::SliceTo(__int64, __int64)
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: mantaroh, Assigned: roc)
References
()
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 1 obsolete file)
24.79 KB,
text/x-log
|
Details | |
40 bytes,
text/x-review-board-request
|
jya
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
This bug was filed from the Socorro interface and is report bp-e3c64afb-843a-4170-a9e5-59a152151014. ============================================================= Reproduce step: 1) access the testpage[1]. 2) select ogv file. Actually Result: Crash (non e10s)
Reporter | ||
Comment 1•9 years ago
|
||
Sorry, I have filed an unfinished bug. Reproduce Step: 1) access the testpage[1] 2) select ogv file. Actually Result: - Crashed the firefox.(none e10s) - Crashed the tab.(e10s) Expected Result: we can play the video without crash. [1] http://mantaroh.github.io/crash-audio-chunk/
Reporter | ||
Comment 2•9 years ago
|
||
Hi cpearce, I tried to play ogg video using blob and mozCaptureStream. However current nightly was crashed. (It was success using video's src attribute.) Some ogg video was success. I have a question about ogg file format. Does support ogg video using blob in current Firefox? Additional Info: In fact current B2G Simulator for TV is using similar implementation, And this simulator have similar crash bug.(bug 1214115)
Flags: needinfo?(cpearce)
Comment 3•9 years ago
|
||
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2) > Does support ogg video using blob in current Firefox? Should do. Looks like this is a crash in MSG code, not in Ogg specific code.
Flags: needinfo?(cpearce)
Reporter | ||
Comment 4•9 years ago
|
||
Thanks Chris. I tested nightly with debug symbols. It seems MediaStreamGraphImpl::UpdateCurrentTimeForStreams() accessed the released stream object.[1] I don't know why we are accessing a released object. I also tried testing with a debug build. The assertion at TrackUnionStream:CopyTrackData[2] failed. I have attached a console log. I'm not familiar with MediaStreamGraph. Can you suggest someone who can help? [1] https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/dom/media/MediaStreamGraph.cpp#245 [2] https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/dom/media/TrackUnionStream.cpp#270
Attachment #8674639 -
Flags: review?(cpearce)
Reporter | ||
Updated•9 years ago
|
Attachment #8674639 -
Flags: review?(cpearce)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
Comment 5•9 years ago
|
||
Roc or JW know more about the MSG and may be able to help.
Flags: needinfo?(roc)
Flags: needinfo?(jwwang)
Flags: needinfo?(cpearce)
Comment 6•9 years ago
|
||
Haven't touch MSG for a while. Paul should know better.
Flags: needinfo?(jwwang) → needinfo?(padenot)
Comment 7•9 years ago
|
||
Andreas, this is asserting like crazy in tracks code, care to have a look? This is easy to repro here. You open the URL, save the two files locally, and load them using the button, alternating between the two (sometimes it works, and I find sometimes shift reloading helps repro-ing).
Flags: needinfo?(pehrsons)
Comment 8•9 years ago
|
||
I'm a bit swamped with MediaStream stuff at the moment, but I'll put it on my list of things to do while waiting for reviews.
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 10•9 years ago
|
||
We get into DecodedStream::SendVideo with mInfo.mAudio.mTrackId == mInfo.mVideo.mTrackId == -1. This actually comes from OggReader. In OggReader::ReadMetadata mInfo ends up with those two mTrackIds both being -1. They definitely shouldn't have the same track ID, and I guess they shouldn't be TRACK_INVALID.
Assignee | ||
Comment 11•9 years ago
|
||
Yeah, so OggReader simply doesn't assign track IDs. So duplicate TRACK_INVALID track IDs get put into the MediaStream and things go downhill from there.
Assignee | ||
Comment 12•9 years ago
|
||
Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya
Attachment #8681038 -
Flags: review?(jyavenard)
Assignee | ||
Comment 13•9 years ago
|
||
Bug 1214506. Eliminate default values for track IDs. r=jya
Attachment #8681039 -
Flags: review?(jyavenard)
Comment 14•9 years ago
|
||
Comment on attachment 8681038 [details] MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya https://reviewboard.mozilla.org/r/23721/#review21245 ::: dom/media/ogg/OggReader.cpp:126 (Diff revision 1) > - aEnable); > + aEnable, (TrackID)aTrackType); This is a bit ugly IMHO. What does casting a TrackType into a TrackID means? And a C-cast at that :) If we want to set the track id for audio to 1 and video track id to 2 ; It would be better to have this set explicitly. eg. aTrackType == TrackType::kAudioTrack ? ... enum TrackType { kUndefinedTrack, kAudioTrack, kVideoTrack, kTextTrack }; the value of kAudioTrack and kVideoTrack should be valueless. If anyone add a constant before kAudioTrack, the OggReader will then be broken. I think it would be safer to have a pure virtual TrackInfo::Init that doesn't take a TrackType. It seems wrong to me to be able to run Init on an existing VideoTrack and be able to change its type. This would cause havoc in the MediaCode as we typically pass a const TrackInfo& and rely on the TrackType value to cast this into either a VideoInfo& or an AudioInfo&. We would then likely perform an out of bound read
Attachment #8681038 -
Flags: review?(jyavenard)
Comment 15•9 years ago
|
||
Comment on attachment 8681039 [details] MozReview Request: Bug 1214506. Eliminate default values for track IDs. r=jya https://reviewboard.mozilla.org/r/23723/#review21249
Attachment #8681039 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #14) > ::: dom/media/ogg/OggReader.cpp:126 > (Diff revision 1) > > - aEnable); > > + aEnable, (TrackID)aTrackType); > > This is a bit ugly IMHO. What does casting a TrackType into a TrackID means? > And a C-cast at that :) It doesn't matter as long as it gives us unique values. > If we want to set the track id for audio to 1 and video track id to 2 ; It > would be better to have this set explicitly. > eg. > aTrackType == TrackType::kAudioTrack ? ... > > enum TrackType { > kUndefinedTrack, > kAudioTrack, > kVideoTrack, > kTextTrack > }; > > the value of kAudioTrack and kVideoTrack should be valueless. I don't know what this means. > If anyone add a constant before kAudioTrack, the OggReader will then be > broken. I don't know what this means either. > I think it would be safer to have a pure virtual TrackInfo::Init that > doesn't take a TrackType. > It seems wrong to me to be able to run Init on an existing VideoTrack and be > able to change its type. I can just remove the aTrackType parameter from Init.
Comment 17•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > > This is a bit ugly IMHO. What does casting a TrackType into a TrackID means? > > And a C-cast at that :) > > It doesn't matter as long as it gives us unique values. I thought you wanted them to be of a specific value (here: audio=1 and video=2) which made sense as it's the default values given by AudioInfo and VideoInfo constructor respectively. > > If anyone add a constant before kAudioTrack, the OggReader will then be > > broken. > > I don't know what this means either. If it's the value 1 and 2 that we need; adding an enum in that declaration would change it to 2 and 3 etc. > > > I think it would be safer to have a pure virtual TrackInfo::Init that > > doesn't take a TrackType. > > It seems wrong to me to be able to run Init on an existing VideoTrack and be > > able to change its type. > > I can just remove the aTrackType parameter from Init. that would work. It appears this is only used by OggReader however. so maybe it doesn't matter.
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8681038 [details] MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya
Attachment #8681038 -
Flags: review?(jyavenard)
Assignee | ||
Updated•9 years ago
|
Attachment #8681039 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
Comment on attachment 8681038 [details] MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya https://reviewboard.mozilla.org/r/23721/#review21257
Attachment #8681038 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/485a23bbf84793dd05268420c61642f3856de5a3 Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya
Comment 21•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/485a23bbf847
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 23•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/485a23bbf847
status-b2g-v2.5:
--- → fixed
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8681038 [details] MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya Approval Request Comment [Feature/regressing bug #]: none [User impact if declined]: possible crashes mixing Ogg files with new stream APIs [Describe test coverage new/current, TreeHerder]: automated tests catch this (with asserts added in this patch) [Risks and why]: very low risk, simple change. [String/UUID change made/needed]: none.
Attachment #8681038 -
Flags: approval-mozilla-aurora?
status-firefox44:
--- → affected
Comment on attachment 8681038 [details] MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya This has been on Nightly for over a week, seems safe to uplift to Aurora44.
Attachment #8681038 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 26•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7ab2f17e377
Comment 27•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d7ab2f17e377
You need to log in
before you can comment on or make changes to this bug.
Description
•