Closed Bug 1417300 Opened 2 years ago Closed 2 years ago

Ogg Vorbis streams from Icecast stop after first song played

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: rmcauley, Assigned: alwu, NeedInfo)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
My website, Rainwave, outputs all <audio> events to the console.

Firefox 56 did not exhibit this behavior.
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
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
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
Ever confirmed: true
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Assignee: jyavenard → alwu
Will take a look.
Alastor, any updates?
Flags: needinfo?(alwu)
Sorry, I'm busy on other stuffs recently, I'll start on this today.
Flags: needinfo?(alwu)
Have found the solution, now I'm thinking about how to test this issue.
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+
(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)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29d79d92ef13
adjust sample time before calculating the total decoded duration. r=jya
https://hg.mozilla.org/mozilla-central/rev/29d79d92ef13
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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+
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 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 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 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+
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2663f06d492f
store mDecodedAudioDuration before adjusting sample time. r=jya
https://hg.mozilla.org/mozilla-central/rev/2663f06d492f
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Flags: needinfo?(rmcauley)
Could you please verify if the next Nightly is working for you?

thank you
Flags: needinfo?(phazon)
(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.
(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.
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+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.