Ogg Vorbis streams from Icecast stop after first song played

VERIFIED FIXED in Firefox 58
(NeedInfo from)

Status

()

Core
Audio/Video: Playback
P1
normal
VERIFIED FIXED
2 months ago
29 days ago

People

(Reporter: Rob, Assigned: alwu, NeedInfo)

Tracking

({regression})

57 Branch
mozilla59
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 wontfix, firefox58 verified, firefox59 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

1. Open e.g. http://rainwave.cc
2. Click Play
3. Wait for the next song to start


Actual results:

Next song does not play


Expected results:

Next song should play
(Reporter)

Comment 1

2 months ago
My website, Rainwave, outputs all <audio> events to the console.

Firefox 56 did not exhibit this behavior.
(Reporter)

Comment 2

2 months ago
I've deployed a workaround to sniff the useragent for Firefox to my website so users do not experience this bug, by forcing Firefox 57 to use the MP3 stream.

You can see the issue by opening one of my Ogg streams directly: http://relay-chi.gameowls.com/game.ogg

Updated

2 months ago
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core

Updated

2 months ago
Keywords: regression, regressionwindow-wanted

Comment 3

2 months ago
Build ID 	20171115100050
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

I can reproduce the probleb with http://rainwave.cc.

I checked 3 times each build to find regression window. (because the problem is not reproduce occasionally).

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3cb1ce131f7cab332f036cecd59e5d104c981fa8&tochange=69fec8b3a6a0dc196f2ff465259e9587182c389f

Regressed by: Bug 1398139

@:alwu, @:jya

Your bunch of patch seems to cause the regression, can you look into this?

(:alwu is not accept ni, so insted :jya)
Blocks: 1398139
Status: UNCONFIRMED → NEW
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox59: --- → affected
Ever confirmed: true
Flags: needinfo?(jyavenard)
Keywords: regressionwindow-wanted
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Priority: -- → P1
Assignee: jyavenard → alwu
(Assignee)

Comment 4

2 months ago
Will take a look.
Alastor, any updates?
status-firefox57: affected → fix-optional
Flags: needinfo?(alwu)
(Assignee)

Comment 6

2 months ago
Sorry, I'm busy on other stuffs recently, I'll start on this today.
Flags: needinfo?(alwu)
(Assignee)

Comment 7

2 months ago
Have found the solution, now I'm thinking about how to test this issue.
Comment hidden (mozreview-request)

Comment 9

2 months ago
mozreview-review
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

https://reviewboard.mozilla.org/r/204178/#review209754

::: commit-message-2aeab:3
(Diff revision 1)
> +Bug 1417300 - adjust sample time before calculating the total decoded duration.
> +
> +The sample time of new chained ogg file would start from zero, for keeping timestamp

I can't make sense of what you're trying to say :)

What about:

We do not want to perform the adjustment of the timestamp after reading the ogg chain but before. Otherwise the parent's mDecodedAudioDuration would be adjusted causing the sample's time to be twice the value it should be.

::: dom/media/ogg/OggDemuxer.cpp:1351
(Diff revision 1)
>      return nullptr;
>    }
>    if (mType == TrackInfo::kAudioTrack) {
>      data->mTrackInfo = mParent->mSharedAudioTrackInfo;
>    }
> +  // Adjust the sample time if the bitstream is chained.

please expand on that comment, can add the commit description in there.
Attachment #8933240 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> I can't make sense of what you're trying to say :)
> 
> What about:
> 
> We do not want to perform the adjustment of the timestamp after reading the
> ogg chain but before. Otherwise the parent's mDecodedAudioDuration would be
> adjusted causing the sample's time to be twice the value it should be.

OK, thank you!
Add NI for uplifting this change to 58.
Flags: needinfo?(alwu)

Comment 14

2 months ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29d79d92ef13
adjust sample time before calculating the total decoded duration. r=jya

Comment 15

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29d79d92ef13
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
status-firefox57: fix-optional → wontfix
status-firefox-esr52: --- → unaffected
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Approval Request Comment
[Feature/Bug causing the regression]: bug1398139
[User impact if declined]: the website using chained ogg would be broken
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: 
1. open http://relay-chi.gameowls.com/game.ogg
2. check whether it can play multiple different songs
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affect chained ogg file
[String changes made/needed]: No
Flags: needinfo?(alwu)
Attachment #8933240 - Flags: approval-mozilla-beta?
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Fix a broken chained ogg playback issue. Beta58+.
Attachment #8933240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

a month ago
Is this really fixed? I use Nightly 59.0a1 (2017-12-07) and both http://rainwave.cc/ and http://relay-chi.gameowls.com/game.ogg plays for me exactly TWO songs in row and on third they both become silent. Stoping and restarting playback gives me again two songs and silence after that.
Using 59.0a1 (2017-12-07) 64 bit on Xubuntu 17.10
confirmed.. this is *not* fix.
Status: RESOLVED → REOPENED
Flags: needinfo?(alwu)
Resolution: FIXED → ---
Hmm, indeed, I'll investigate it again.
Sorry for the inconvenience.
Flags: needinfo?(alwu)
Comment hidden (mozreview-request)
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Hi, jya,
Could you help me review this patch again?
I've confirm that the the file [1] can be playback well with chaining over 10+ files.
Thanks!

[1] http://relay-chi.gameowls.com/game.ogg
Attachment #8933240 - Flags: review+ → review?(jyavenard)

Comment 24

a month ago
mozreview-review
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

https://reviewboard.mozilla.org/r/204178/#review212234

::: dom/media/ogg/OggDemuxer.cpp:728
(Diff revision 3)
>    }
>  
>    if (chained) {
>      SetChained();
>      mInfo.mMediaSeekable = false;
> -    mDecodedAudioDuration += aLastEndTime;
> +    mDecodedAudioDuration = aLastEndTime;

this is starting to become confusing, as the argument name no longer match what they were doing.

That, or add comments above, or rename the argument name.

I would prefer you either:
1- rename the argument name to be more explicit on what it is.

2- in OggTrackDemuxer::NextSample(), save mParent->mDecodedAudioDuration; in a temp variable so that it is not affected

e.g.

  // We do not want to perform the adjustment of the timestamp after reading
  // the ogg chain but before. Otherwise the parent's mDecodedAudioDuration
  // would be adjusted causing the sample's time to be twice the value it
  // should be.
  data->mTime += mParent->mDecodedAudioDuration;
  if (eos) {
    // We've encountered an end of bitstream packet; check for a chained
    // bitstream following this one.
    // This will also update mSharedAudioTrackInfo.
    mParent->ReadOggChain(data->GetEndTime());
  }


become:

  
  // mDecodedAudioDuration gets adjusted during ReadOggChain. 
  TimeUnit totalDuration = mParent->mDecodedAudioDuration;
  if (eos) {
    // We've encountered an end of bitstream packet; check for a chained
    // bitstream following this one.
    // This will also update mSharedAudioTrackInfo.
    mParent->ReadOggChain(data->GetEndTime());
  }

  // We adjust the start time of the sample to account for the potential ogg chaining.
  data->mTime += totalDuration;
Attachment #8933240 - Flags: review?(jyavenard) → review-
Comment hidden (mozreview-request)

Comment 26

a month ago
mozreview-review
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

https://reviewboard.mozilla.org/r/204178/#review212512

::: commit-message-2aeab:4
(Diff revisions 3 - 4)
> -Bug 1417300 -  change mDecodedAudioDuration to the last sample time.
> +Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.
>  
> -Since we change the last time before calculating the mDecodedAudioDuration, the last time
> -has been adjusted to the correct timestamp, we don't need to accumulate it.
> +mDecodedAudioDuration should only save unadjusted timestamp, or it would
> +cause next adjustment incorrect.

next adjustment to be incorrect
Attachment #8933240 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)

Comment 28

a month ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2663f06d492f
store mDecodedAudioDuration before adjusting sample time. r=jya

Comment 29

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2663f06d492f
Status: REOPENED → RESOLVED
Last Resolved: 2 months agoa month ago
Resolution: --- → FIXED
Flags: needinfo?(rmcauley)
Could you please verify if the next Nightly is working for you?

thank you
Flags: needinfo?(phazon)

Comment 31

a month ago
(In reply to Jean-Yves Avenard [:jya] from comment #30)
> Could you please verify if the next Nightly is working for you?
> 
> thank you

I guess its not yet arrived in Nightly (my current is latest 59.0a1 (2017-12-10)) so picture is still same. I will recheck on next update.

Comment 32

a month ago
(In reply to Jean-Yves Avenard [:jya] from comment #30)
> Could you please verify if the next Nightly is working for you?
> 
> thank you

(after another Nightly update) Yes, i listened quite extensive and its now works completely fine. Thanks for hard working.
Flags: needinfo?(phazon)
The patch was updated since uplifted to beta, and has now been verified.
Do we need another uplift request or can we push this to beta directly?
Flags: needinfo?(ryanvm)
New request please.
status-firefox58: fixed → affected
Flags: needinfo?(ryanvm) → needinfo?(alwu)
I can't clear the approval flag, so just fill the request comment again.

Approval Request Comment
[Feature/Bug causing the regression]: bug1398139
[User impact if declined]: the website using chained ogg would be broken
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, see comment 32
[Needs manual test from QE? If yes, steps to reproduce]: 
1. open http://relay-chi.gameowls.com/game.ogg
2. check whether it can play multiple different songs
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affect chained ogg file
[String changes made/needed]: No
Flags: needinfo?(alwu) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(gchang)
Attachment #8933240 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Fix a broken playback issue that website uses chained ogg. Beta58+.
Flags: needinfo?(gchang)
Attachment #8933240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 37

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2c6da9d74cda
status-firefox58: affected → fixed
Flags: qe-verify+
I reproduced this issue on both http://rainwave.cc and http://relay-chi.gameowls.com/game.ogg, using 59.0a1 (Build Id: 20171115100050) on Win 10 x64 and using 59.a01 (Build Id: 20171207100053) on Ubuntu 16.04 x64, but I was able to reproduced it only on http://relay-chi.gameowls.com/game.ogg when using the 57.0 (Build ID: 20171112125346) on Windows 10 x64.

This issue is no longer reproducible on 59.0a1(Build Id: 20171219100203) and 58.0b12 (Build Id: 20171218174357) on Windows 10 X64, Ubuntu 16.04 x64 and macOS 10.12 (playing multiple songs in a row)
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.