1417300 - Ogg Vorbis streams from Icecast stop after first song played

Status: VERIFIED FIXED

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

Description

[Rob]

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

Comment 1

[Rob]

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

Firefox 56 did not exhibit this behavior.

Comment 2

[Rob]

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

Comment 3

[Alice0775 White]

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)

Comment 4

[Alastor Wu [:alwu]]

Will take a look.

Comment 5

[Mike Taylor [:miketaylr]]

Alastor, any updates?

Comment 6

[Alastor Wu [:alwu]]

Sorry, I'm busy on other stuffs recently, I'll start on this today.

Comment 7

[Alastor Wu [:alwu]]

Have found the solution, now I'm thinking about how to test this issue.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

The sample time of new chained ogg file would start from zero, for keeping timestamp
increase consistently, we would add the previous total decoded duration to the
sample time.

When the sample is the EOS packet, it means we might end this file and prepare
to chain another file. Then, we calulate total decoded duration, adding the total
duration of this file.

The total decoded duration is used for next chain file, so we should adjust the
time before calculating it.

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

Comment 9

[Jean-Yves Avenard [:jya]]

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.

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204178/diff/1-2/

Comment 11

[Alastor Wu [:alwu]]

(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!

Comment 12

[Alastor Wu [:alwu]]

Add NI for uplifting this change to 58.

Comment 13

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f9d2102f980fea168dccd81363b39821ff8d84&selectedJob=148729179

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29d79d92ef13
adjust sample time before calculating the total decoded duration. r=jya

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/29d79d92ef13

Comment 16

[Alastor Wu [:alwu]]

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

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Fix a broken chained ogg playback issue. Beta58+.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/41aed9dafb97

Comment 19

[V. Korn]

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

Comment 20

[Jean-Yves Avenard [:jya]]

confirmed.. this is *not* fix.

Comment 21

[Alastor Wu [:alwu]]

Hmm, indeed, I'll investigate it again.
Sorry for the inconvenience.

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204178/diff/2-3/

Comment 23

[Alastor Wu [: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

Comment 24

[Jean-Yves Avenard [:jya]]

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;

Comment 25

[Alastor Wu [:alwu]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204178/diff/3-4/

Comment 26

[Jean-Yves Avenard [:jya]]

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

Comment 27

[Alastor Wu [:alwu]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204178/diff/4-5/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2663f06d492f
store mDecodedAudioDuration before adjusting sample time. r=jya

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2663f06d492f

Comment 30

[Jean-Yves Avenard [:jya]]

Could you please verify if the next Nightly is working for you?

thank you

Comment 31

[V. Korn]

(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

[V. Korn]

(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.

Comment 33

[Jean-Yves Avenard [:jya]]

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?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

New request please.

Comment 35

[Alastor Wu [: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

Comment 36

[Gerry Chang [:gchang]]

Comment on attachment 8933240 [details]
Bug 1417300 - store mDecodedAudioDuration before adjusting sample time.

Fix a broken playback issue that website uses chained ogg. Beta58+.

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2c6da9d74cda

Comment 38

[Bogdan Maris, Desktop QA]

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)