Closed
Bug 1265794
Opened 8 years ago
Closed 8 years ago
Video playback is broken on the web
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: gerard-majax, Assigned: jya)
References
Details
Attachments
(1 file)
STR: - build and flash pine or m-c - try to play a video from youtube Expected: I can see the video Actual: Video never starts Reproduced also on m-c. No problem on both for audio as much as I could test.
Comment 1•8 years ago
|
||
I also faced the problem on master aries. I am looking into what causes the problem.
Updated•8 years ago
|
Component: General → Audio/Video: Playback
Product: Firefox OS → Core
Comment 2•8 years ago
|
||
Audio decoding caused the problem. GonkAudioDecoderManager::CreateAudioData() did not exit. In AudioCompactor::Push(), it di infinite loop. At first aFrames was 1024, then it became 16. But since aFrames == 16, it kept same value forever. The following set samples to 1. And it made framesCopied to 0(no copy) since 2nd loop. > uint32_t samples = GetChunkSamples(aFrames, aChannels, maxSlop); > if (aFrames * aChannels > mSamplesPadding) { > samples -= mSamplesPadding; > } https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioCompactor.h?from=AudioCompactor#51
Comment 3•8 years ago
|
||
mSamplesPadding was added by bug 1248861.
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #2) > Audio decoding caused the problem. > GonkAudioDecoderManager::CreateAudioData() did not exit. In > AudioCompactor::Push(), it di infinite loop. At first aFrames was 1024, then > it became 16. But since aFrames == 16, it kept same value forever. > > The following set samples to 1. And it made framesCopied to 0(no copy) since > 2nd loop. mSamplesPadding will be 62 by default. If aFrames is 16, and I assume aChannels is 2. > > > uint32_t samples = GetChunkSamples(aFrames, aChannels, maxSlop); this would return 32 > > if (aFrames * aChannels > mSamplesPadding) { this is false as 16 * 2 = 32 which is < 62 > > samples -= mSamplesPadding; > > } > > https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioCompactor. > h?from=AudioCompactor#51 so I can't see how the behaviour you described could happen. the gtests testing AudioCompactor are obviously wrong then because it passes all the gtest. I'll have a look to see what's going when aFrames is 16.
Comment 5•8 years ago
|
||
mSamplesPadding was 31 with the following videos on master aries. http://people.mozilla.org/~cpeterson/videos/H264_Baseline_Profile_Level_30_640x360p.mp4 At the following, paddedSize was 62. And mSamplesPadding was 31. > size_t paddedSize = AlignedAudioBuffer::AlignmentPaddingSize(); > mSamplesPadding = paddedSize / sizeof(AudioDataValue);
Assignee | ||
Comment 6•8 years ago
|
||
can you replace: > > if (aFrames * aChannels > mSamplesPadding) { with: > > if (samples > mSamplesPadding) { and see if that fixes the problem for you?
Assignee | ||
Comment 7•8 years ago
|
||
Make that: if (samples / aChannels > mSamplesPadding / aChannels + 1) { patch on the way That also means that gtests do not run on Android / B2G which is kind of a shame as the AudioCompactor is only used on those devices.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jyavenard
Comment 8•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #7) > Make that: > if (samples / aChannels > mSamplesPadding / aChannels + 1) { > patch on the way Yea, I confirmed, it addressed the problem. Thanks!
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47645/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47645/
Attachment #8743211 -
Flags: review?(giles)
Assignee | ||
Comment 10•8 years ago
|
||
The problems come that mSamplesPadding was odd and we could encounter the case where int((num_samples - mSamplesPadding) / channels) = 0 where num_samples is frames * channel. This can be reproduced easily if sizeof(AudioDataValue) is 2 (just need to have a remainder == 16) I tried to come up with a gtest that would reproduce the issue on desktop where gtest are properly run, but on desktop sizeof(AudioDataValue) is 4 and that doesn't easily allow to find an edge case. I'm fairly confident that there is, but on top of my head I can't find one. So this will do for now.
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #9) > Created attachment 8743211 [details] > MozReview Request: Bug 1265794: P1. Ensure we can always fit a complete > audio frame in an audio buffer. r?rillian > > Review commit: https://reviewboard.mozilla.org/r/47645/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/47645/ I first ran into a failure trying to play a video after applying this patch, but then it worked. 04-20 13:38:46.888 4845 4863 D MediaCodecProxy: resourceReserved 04-20 13:38:46.888 4845 5029 I OMXClient: Using client-side OMX mux. 04-20 13:38:46.888 4845 5030 I OMXClient: Using client-side OMX mux. 04-20 13:38:46.898 4845 5029 E OMXMaster: A component of name 'OMX.qcom.audio.decoder.aac' already exists, ignoring this one. 04-20 13:38:46.908 4845 5025 D GonkAudioDecoderManager: Configure audio mime type:audio/mp4a-latm, chan no:2, sample-rate:48000, profile:2 04-20 13:38:46.918 4845 5025 D GonkAudioDecoderManager: Failed to input codec specific data! 04-20 13:38:47.058 4845 4845 W FRANCE 24: [JavaScript Warning: "Media resource http://medias.france24.com/fr/ptw/2016/04/20/NW821120-A-01-20160420.mp4 could not be decoded." {file: "http://medias.france24.com/fr/ptw/2016/04/20/NW821120-A-01-20160420.mp4" line: 0}] 04-20 13:38:47.088 297 715 E : Destroy C2D instance 04-20 13:38:47.088 297 715 E : Destroy C2D instance 04-20 13:38:47.118 4845 4845 E FRANCE 24: [JavaScript Error: "TypeError: this.controlListeners is undefined" {file: "chrome://global/content/bindings/videocontrols.xml" line: 699}]
Comment 12•8 years ago
|
||
Comment on attachment 8743211 [details] MozReview Request: Bug 1265794: P1. Ensure we can always fit a complete audio frame in an audio buffer. r?rillian https://reviewboard.mozilla.org/r/47645/#review44539
Attachment #8743211 -
Flags: review?(giles) → review+
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8f1a46ea42a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•