1285928 - No sound when playing video in Standard Definition quality mode

Status: VERIFIED FIXED

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

Description

[roxleitan]

20160623154057 Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
20160710030217 Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

[Affected versions]:
Release candidate: 47.0.1
Nightly: 50.0a1 

[Affected platforms]:
Windows - all

[Steps to reproduce]:
1.Open FF with Clean Profile
2.Open http://trailers.apple.com/trailers/wb/batmanthekillingjoke/
3.Click Play

[Expected result]:
Video should play properly(audio and video)

[Actual result]:
Video is playing without sound

[Additional notes]:
When video quality is changed to High Definition 720p or High Definition 1080p video is playing properly

Comment 1

[Alice0775 White]

Regression window for audio broken:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=508edecf64bb&tochange=31b08880d272

Suspect: Bug 1168040

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1285928: [mp4] Ignore non-supported entries in edit list.

Only the first entry in the edit list is supported (or the 2nd if the first entry is empty)

Review commit: https://reviewboard.mozilla.org/r/63620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63620/

Comment 3

[Jean-Yves Avenard [:jya]]

those files contains multiple entries in their edit list which stagefright doesn't support. A logic error caused the 2nd entry to be used and overwrite the first.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8769991 [details]
Bug 1285928: [mp4] Ignore non-supported entries in edit list.

https://reviewboard.mozilla.org/r/63620/#review61482

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:880
(Diff revision 1)
>                      if (!mLastTrack) {
>                        return ERROR_MALFORMED;
>                      }
>                      mLastTrack->empty_duration = segment_duration;
>                      continue;
> -                } else if (i > 1) {
> +                } else if (nonEmptyCount >= 1) {

Given that nonEmptyCount can only ever be 0 or 1 you should just make it bool seenNonEmpty and change nonEmptyCount++ to seenNonEmpty=true

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc189ba3703d
[mp4] Ignore non-supported entries in edit list. r=kentuckyfriedtakahe

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc189ba3703d

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Jean-Yves (or Anthony) please request uplift approval as appropriate.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8769991 [details]
Bug 1285928: [mp4] Ignore non-supported entries in edit list.

Approval Request Comment
[Feature/regressing bug #]: 1168040
[User impact if declined]: Some files won't play
[Describe test coverage new/current, TreeHerder]: in central (now beta) for a few weeks
[Risks and why]: Low. We ignore edit list entry that we can't process anyway.
[String/UUID change made/needed]: none

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8769991 [details]
Bug 1285928: [mp4] Ignore non-supported entries in edit list.

This already shows fixed in 50, switching the flag to beta:?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8769991 [details]
Bug 1285928: [mp4] Ignore non-supported entries in edit list.

We want audio to work for Windows users, let's uplift to beta and verify from there.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, since Roxana is on PTO, can someone else from your team verify this on beta? Thanks!

Comment 12

[Andrei Vaida [:avaida]]

I've managed to reproduce the bug on an affected build: 47.0.1-build1 
(20160623154057), using Windows 10 x64.

The latest beta tinderbox build I found [1] is still showing the bug,
but I'll check again in the following days to make sure -- leaving
the ni? in place until then.

This is verified fixed on 50.0a2 (2016-08-10), though.


[1] 20160809191615 
    https://hg.mozilla.org/releases/mozilla-beta/rev/0ae3dac31645

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff14a84489e5

Comment 14

[Andrei Vaida [:avaida]]

This is verified fixed on 49.0b3-build1 (20160811031722) as well,
using Windows 10 x64.

Comment 15

[Ralph Giles (:rillian)]

Alfredo, this is a compatibility issue we should check.

Comment 16

[Alfredo Yang (:alfredo)]

(In reply to Ralph Giles (:rillian) needinfo me from comment #15)
> Alfredo, this is a compatibility issue we should check.

Rust parser is ok at this case, it uses the first entry in elst.