Closed Bug 1265794 Opened 8 years ago Closed 8 years ago

Video playback is broken on the web

Categories

(Core :: Audio/Video: Playback, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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.
I also faced the problem on master aries. I am looking into what causes the problem.
Component: General → Audio/Video: Playback
Product: Firefox OS → Core
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
Blocks: 1248861
mSamplesPadding was added by bug 1248861.
(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.
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);
can you replace:

> >      if (aFrames * aChannels > mSamplesPadding) {

with:
> >      if (samples > mSamplesPadding) {

and see if that fixes the problem for you?
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: nobody → jyavenard
(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!
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.
(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 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+
https://hg.mozilla.org/mozilla-central/rev/b8f1a46ea42a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.