Last Comment Bug 455165 - Firefox fails on chained ogg stream
: Firefox fails on chained ogg stream
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86 All
: -- critical with 19 votes (vote)
: mozilla20
Assigned To: Paul Adenot (:padenot)
:
: Maire Reavy [:mreavy]
Mentors:
http://www.ruecker-itk.de/icecast/fir...
: 494504 509438 517190 559314 611519 738990 (view as bug list)
Depends on: 531340 793315 819150 845790
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-13 13:21 PDT by Thomas B. Rücker [account no longer active]
Modified: 2013-07-06 02:15 PDT (History)
44 users (show)
roc: wanted1.9.1-
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Part 1 -- Change return value for Decode* methods in nsBuiltinDecoderStateMachine (23.16 KB, patch)
2011-08-08 10:47 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2 -- WIP, implements audio-only ogg file uninterupted playing. (20.61 KB, patch)
2011-08-08 10:55 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 1 -- Change return value for Decode* methods in nsBuiltinDecoderStateMachine (23.12 KB, patch)
2011-11-26 09:38 PST, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2 -- Implements audio-only chained OGG playback. (17.96 KB, patch)
2011-11-26 09:40 PST, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 1 - Change the return value type of Decode* methods in all backends r= (33.07 KB, patch)
2012-08-06 18:20 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2 - Add support for chained ogg streams, and metadata updating. r= (26.47 KB, patch)
2012-08-06 18:29 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Firefox Vorbis Chaining Patch (2.38 KB, patch)
2012-08-21 02:54 PDT, David Richards
no flags Details | Diff | Splinter Review
Part 1 - Change the return value type of Decode* methods in all backends r= (37.39 KB, patch)
2012-09-14 11:40 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2 - Add support for chained ogg streams, and metadata updating. r= (32.94 KB, patch)
2012-09-14 11:44 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 3 - Test chaining with various files. r= (237.67 KB, patch)
2012-09-14 11:47 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 1 - Change the return value type of Decode* methods in all backends r= (37.49 KB, patch)
2012-09-24 10:31 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Splinter Review
Part 2 - Add support for chained ogg streams, and metadata updating. r= (33.56 KB, patch)
2012-09-24 10:32 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Splinter Review
Part 3 - Test chaining with various files. r= (348.29 KB, patch)
2012-09-24 10:34 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Minimalist Ogg Chaining Fix (3.19 KB, patch)
2012-10-05 05:08 PDT, David Richards
no flags Details | Diff | Splinter Review
Add support for chained ogg audio file and proper metadata. r= (38.10 KB, patch)
2012-11-26 05:13 PST, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Splinter Review
Tests for ogg chain support. r= (282.17 KB, patch)
2012-11-26 05:16 PST, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Splinter Review
Put some very verbose media logging behind a MOZ_QUIET=1 check. r= (2.87 KB, patch)
2012-11-29 07:07 PST, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Splinter Review
Review comments. Should be folded before landing. (55.49 KB, patch)
2012-11-29 07:29 PST, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Splinter Review
Tests for ogg chain support. r= (416.97 KB, patch)
2012-11-29 07:30 PST, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add support for chained ogg audio file and proper metadata dispatching. r= (63.02 KB, patch)
2012-11-29 07:33 PST, Paul Adenot (:padenot)
akeybl: approval‑mozilla‑b2g18-
Details | Diff | Splinter Review

Description Thomas B. Rücker [account no longer active] 2008-09-13 13:21:57 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2) Gecko/20080829071937 Shiretoko/3.1a2

Besides being a bumpy ride playback ultimately fails when a switch to the next chained part occurs.

Reproducible: Always

Steps to Reproduce:
1. open URL in Fx3.1a2
2. wait for an segment to end.
3. playback freezes. Stream is still being pulled from the icecast server.
Actual Results:  
Playback freezes (both audio and video).
Bandwith is still being consumed due to continued streaming of the content.
Toggling play/stop/pause does not help.

Expected Results:  
Seamless continued playback.

This probably also applies to chained static ogg files.

I'll modify the example html to point to an stream with more frequent chaining events later today. This will make it easier to reproduce.
Comment 1 cajbir (:cajbir) 2008-09-13 17:30:34 PDT
Yes, this is a known liboggplay issue - it doesn't currently support chained ogg files. When it does, we'll pick it up.
Comment 2 cmtalbert 2008-09-15 07:18:33 PDT
From comment 1, marking invalid as this is not an issue in the Firefox side of the video/audio support
Comment 3 Christopher Blizzard (:blizzard) 2009-01-12 11:44:25 PST
Re-opening.  We'll still need to get a fix for it once upstream fixes it.
Comment 4 Aaron Kaluszka 2009-01-31 10:12:50 PST
Confirming.  The difference I'm seeing is that playing will resume with the next song if you hit play again. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre
Comment 5 Christopher Blizzard (:blizzard) 2009-04-12 06:22:49 PDT
Hmm - did this get fixed?  Something in the back of my brain says that it might have?
Comment 6 cajbir (:cajbir) 2009-04-12 06:31:03 PDT
No it hasn't been fixed.
Comment 7 Christopher Blizzard (:blizzard) 2009-04-12 10:35:46 PDT
Sadfaces.  Going to make 1.9.1?
Comment 8 cajbir (:cajbir) 2009-04-12 18:25:27 PDT
Not very likely. We need support in liboggplay - the changes on our end aren't too big I think.
Comment 9 Christopher Blizzard (:blizzard) 2009-04-12 18:42:53 PDT
Hmm, I thought that was part of the contract we executed?
Comment 10 cajbir (:cajbir) 2009-04-12 19:02:58 PDT
I would have to check - I thought it wasn't but I could be wrong!
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-14 04:24:55 PDT
Realistically, there is no way this is going to happen for 1.9.1 and we have bigger fish to fry anyway.
Comment 12 Matthew Gregan [:kinetik] 2009-09-17 03:59:11 PDT
*** Bug 517190 has been marked as a duplicate of this bug. ***
Comment 13 Aaron Kaluszka 2010-01-21 12:02:27 PST
CC'ing potential implementer for further comment on liboggplay status.

Just FYI, the behavior seems to have changed again.  No longer does pressing play restart the stream, instead it replays cached data.
Comment 14 cajbir (:cajbir) 2010-03-09 13:51:39 PST
*** Bug 509438 has been marked as a duplicate of this bug. ***
Comment 15 Chris Pearce (:cpearce) 2010-04-14 14:39:54 PDT
*** Bug 559314 has been marked as a duplicate of this bug. ***
Comment 16 Thomas B. Rücker [account no longer active] 2010-08-04 03:41:40 PDT
Can we please make this blocking?
This was to be fixed by an new back-end long ago. We can't afford to have yet another major release with broken ogg support!

To restate this: this breaks an use-case predestined for Firefox - playing back streamed music in <audio> elements within websites!
Comment 17 Thomas B. Rücker [account no longer active] 2010-08-05 00:11:57 PDT
Bug 531340 seems to have targeted this with an replacement back-end but I can't seem to get the behavior on Mozilla/5.0 (X11; Linux i686; rv:2.0b2) Gecko/20100720 Firefox/4.0b2
At least I don't even understand if it landed or was just closed uncommitted (cf. my comment there).
Comment 18 Chris Pearce (:cpearce) 2010-08-05 00:55:23 PDT
The new decoder architecture will enable us to implement chaining support, and other backends, more easily. We've not had time to implement ogg chaining yet. It's unlikely we'll get to this ourselves before Firefox 4, we'd happily take a patch...
Comment 19 Matthew Gregan [:kinetik] 2010-11-11 16:50:40 PST
*** Bug 611519 has been marked as a duplicate of this bug. ***
Comment 20 juan becerra [:juanb] 2010-11-11 17:01:56 PST
While looking at bug 611519 I saw about 35 bugs related to "ogg stream" in bugzilla. It looks like this area isn't high in the priority list, but we should at least sort those out.
Comment 21 Chris Pearce (:cpearce) 2010-11-11 17:05:14 PST
It seems likely we'll do something about this post Firefox 4, as we'll probably need most of the underlying infrastructure required for this in order to implement adaptive streaming.
Comment 22 David Richards 2010-11-11 20:25:18 PST
I am sorry about the duplicate bug, I was searching for vorbis rather than "chained ogg" and didn't see this. May I ask, if one was streaming ogg vorbis to firefox now (version 3.6 or 4 for that matter) is it just creating a huge memory leak or is it aware that it is streaming? ie. should this not be done at this time?
Comment 23 Thomas B. Rücker [account no longer active] 2011-07-09 06:54:04 PDT
(In reply to comment #21)
> It seems likely we'll do something about this post Firefox 4, as we'll
> probably need most of the underlying infrastructure required for this in
> order to implement adaptive streaming.

Well we're at 5 now (which is more like 4.1, but anyway).
Firefox continues to violate the RFC3533 http://www.ietf.org/rfc/rfc3533.txt
and breaks listening to streaming audio. It's a real bother. Icecast is a great streaming server and we manage to convince many people to use open standards: http for transport, ogg for encapsulation, vorbis and theora as codecs. 

Still we are forced to tell them: oh you know for everything Gecko based you have to put this ugly ugly hack in place and run a second stream. That's where people either leave or opt for using non free technologies like mp3 and flash.
And people are wondering why http://dir.xiph.org is filled with non-free codec streams.

Please bump this up in priority and start working on it. This bug is approaching its 3rd birthday...
Comment 24 Rob 2011-07-26 21:13:34 PDT
It seems as if liboggplay is a dead project, over a year since its last commit in git, and nothing in the commit logs about chained Ogg.

Would it be terribly difficult to work with the stock libogg instead of liboggplay?  I've worked a bit with the former but the Mozilla codebase intimidates me far too much in order to crack open the code for this bug.

As the admin of the 2nd largest station on dir.xiph.org that runs Vorbis* exclusively, I have to forward that a lot of my users are really demanding support for my station in their browser and on their phones.  Firefox would be perfect for this. (on the desktop, and in Android, where the same problem with chaining appears and no app has yet to fill the gap)

So yes, this is *the* reason Vorbis is simply unused for streaming.  Lack of support.  Firefox is part of the problem. (as are the other browser vendors, with the notable exception of Opera, where Vorbis streaming works fine)

* not all of our relays successfully report to dir.xiph.org, we actually pull 250-300 listeners at any time during the day, making us far and away the largest Vorbis station publically listed on Xiph.
Comment 25 Matthew Gregan [:kinetik] 2011-07-26 21:22:31 PDT
Firefox stopped using liboggplay as of 4.0.
Comment 26 Chris Pearce (:cpearce) 2011-07-26 21:34:55 PDT
Since we've now dropped liboggplay, it would be much easier to add rudimentary ogg chaining playback support. It could be added by altering nsOggReader [1] so that when DecodeAudioData() [2] and DecodeVideoFrame() [3] encountered new header packets after the last eof packet in each "link" in the "chain" was encountered it reinitialized its metadata. This may also require changes to nsBuiltinDecoderStateMachine::DecodeLoop() [4] as it doesn't expect streams which have finished to be reinitialized and start producing data again. See also [5] for an overview of the media decoder architecture.

We'd also need to ensure that seeking did something reasonable (like failing fast) as it's pretty unpleasant implementing seeking in chains.

Patches would be much appreciated... ;)

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggReader.h
[2] http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggReader.cpp#397
[3] http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggReader.cpp#495
[4] http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#312
[5] http://blog.pearce.org.nz/2011/02/firefox-4-video-decoder-architecture.html
Comment 27 Paul Adenot (:padenot) 2011-08-01 07:40:30 PDT
I'm going to work on this, so I've set up a test server [1] which uses 4 seconds OGG chained files, using Icecast2, which I assume is the technology Thomas and Rob are using to power their webradios.

[1] http://paul.cx:8000/example.ogg
Comment 28 Rob 2011-08-01 07:54:00 PDT
Thanks very much Paul.

You can also use any of the URLs here:

http://textville.net:8000/

(ignore the padlocks, the streams will play regardless of login info)
Comment 29 cajbir (:cajbir) 2011-08-01 15:14:06 PDT
Paul, you can also test it by concatenating ogg files together and trying to play the resulting combined one:

cat one.ogg two.ogg three.ogg >combined.ogg

Make sure they are different files so that the serial numbers for the streams are different.
Comment 30 Paul Martin 2011-08-01 15:26:59 PDT
It only plays the first one. (I'm on 6.0b3)

Processing file "/ram/test.ogg"...

New logical stream (#1, serial: 16be823f): type vorbis
Vorbis headers parsed for stream 1, information follows...
Version: 0
Vendor: Xiph.Org libVorbis I 20030909 (1.0.1)
Channels: 1
Rate: 44100

Nominal bitrate: 120.002000 kb/s
Upper bitrate not set
Lower bitrate not set
Vorbis stream 1:
        Total data length: 58814 bytes
        Playback length: 0m:04.592s
        Average bitrate: 102.441258 kb/s
Logical stream 1 ended
New logical stream (#2, serial: 16be8240): type vorbis
Vorbis headers parsed for stream 2, information follows...
Version: 0
Vendor: Xiph.Org libVorbis I 20030909 (1.0.1)
Channels: 1
Rate: 44100

Nominal bitrate: 120.002000 kb/s
Upper bitrate not set
Lower bitrate not set
Vorbis stream 2:
        Total data length: 45908 bytes
        Playback length: 0m:03.210s
        Average bitrate: 114.377718 kb/s
Logical stream 2 ended
New logical stream (#3, serial: 16be8241): type vorbis
Vorbis headers parsed for stream 3, information follows...
Version: 0
Vendor: Xiph.Org libVorbis I 20030909 (1.0.1)
Channels: 1
Rate: 44100

Nominal bitrate: 120.002000 kb/s
Upper bitrate not set
Lower bitrate not set
Vorbis stream 3:
        Total data length: 19947 bytes
        Playback length: 0m:01.586s
        Average bitrate: 100.577421 kb/s
Logical stream 3 ended
New logical stream (#4, serial: 16be8242): type vorbis
Vorbis headers parsed for stream 4, information follows...
Version: 0
Vendor: Xiph.Org libVorbis I 20030909 (1.0.1)
Channels: 1
Rate: 44100
...
Comment 31 Paul Adenot (:padenot) 2011-08-02 11:26:24 PDT
Okay, here is what I plan to do :

- When an eos is found while decoding ogg, set a flag in the state machine to indicates that we should re-read metadata in the stream. This should be done holding the monitor, but it should be acceptable. Then the decode thread should wait.
- When the RunStateMachine is called the next time, call StartDecoder, which is supposed to start by reading the metadata in the ogg stream. We should save these metadata somewhere, since they are about the next stream, and therefore shouldn't be set immediately. Then, the decoder could continue to decode OGG, if needed.
- The audioloop should be informed somehow that it is now playing the new stream (maybe via checking if the current time is lower than the previous current time), and it should get these metadata. I wonder how could be inform the user that the metadata have changed.

This should ensure an uninterrupted playback, but it is kind of a mess in my head right now, and I must obviously be missing something.
Comment 32 Chris Pearce (:cpearce) 2011-08-02 15:59:03 PDT
I discussed this with derf and the ogg/xiph guys on frenode #theora, and the general consensus is that most existing implementations can't handle the case where the Vorbis sample rate changes between "links" in a "chain". So for a first cut implementation we can just dispatch a decode error when the sample rate or number of channels changes between links, and abort playback.

If we do this, we don't need to worry about re-initing {nsBuiltinDecoderStateMachine, nsBuiltinDecoderReader}::mInfo.

Probably best to permanently mark the media as infinite and unseekable once we realise we're playing a chain.

Keep in mind as you implement this that streams in a link aren't guaranteed to end at the same time; the video stream may end some unknown number of seconds after the end of the audio stream, and vice versa.

I'm not very keen on going back into reading metadata state, as that we don't want to stop playback beore restarting the next stream; we want it to be as seemless as possible.

Probably the cleanest way to implement this is to change nsBuiltinDecoderReader::DecodeVideoFrame() and nsBuiltinDecoderReader::DecodeAudioData() to return a quad-state result, rather than a boolean. Something like:

enum DecodeResult {
  MORE_DATA, // Decoding again 
  GAP_IN_DATA,
  FINISHED,
  ERROR // Signal to dispatch decode error to media element
};

If you can think of better names for these states that's fine, but the idea is independent of names of course. ;)

MORE_DATA is the same as Decode*Data() returning PR_TRUE, it means there's more data to come, call decode again for more data.

GAP_IN_DATA means the stream has ended, but there may be playable data in another link in the chain, so call again in future to detect this. When a stream returns GAP_IN_DATA, we treat it as if it's not playing (for things like the skip-to-keyframe calculation and buffering calulations) but we still keep calling Decode*Data() on the stream, as it may swith to MORE_DATA or FINISHED. Media without the concept of chaining need not enter this state, they can go straight to FINISHED. I imagine the decoded audio/video queues will need to report being "finished" in this state for the buffering logic not to misfire (you'll need to check this, I may be wrong).

FINISHED is the same as when Decode*Data() returns PR_FALSE. The stream is done playing. We move to this state when a stream reaches physical EOS.

In their Decode*Data() methods, the WebM/Wave readers then need to return MORE_DATA in place of PR_TRUE, and FINISHED in place of PR_FALSE.

The Ogg reader needs to detect EOS in its Decode*Data() methods. When a stream has reached EOS, but other streams in the same link haven't, that stream's decode method needs to return GAP_IN_DATA. Once all streams have finished (the entire link has been decoded) the decode functions look for header packets of their appropriate type, and re-init their streams, and start producing samples/frames again. Note if headers aren't found it needs to keep returning GAP_IN_DATA; it can't return FINISHED as a subsequent link in the chain may have data of that type. AudioLoop needs to pause its audio stream if it's in GAP_IN_DATA state, but not shut it down.

Frames/samples produced in subsequent links in the chain need to have timestamps relative to the timeline of the first link in the chain, e.g. they need to appear to follow immediately on from the end of the previous link.

May need to alter nsAudioStream somehow so that GetPosition() returns a sensible result in the case when the audio stream of a link finished before its video stream.

If DecodeAudioData() detects a change in the sample rate or number of channels, return ERROR, which dispatches a decode error in DecodeLoop().

Hopefully that makes sense and gives you enough to get started.
Comment 33 Paul Adenot (:padenot) 2011-08-08 10:47:17 PDT
Created attachment 551499 [details] [diff] [review]
Part 1 -- Change return value for Decode* methods in nsBuiltinDecoderStateMachine
Comment 34 Paul Adenot (:padenot) 2011-08-08 10:55:21 PDT
Created attachment 551502 [details] [diff] [review]
Part 2 -- WIP, implements audio-only ogg file uninterupted playing.

Here is a implementation of chained ogg audio files decoding and playing, which seems quite clumsy (and obviously error-prone), but is nevertheless working.

I guess some part of the behavior we want is implemented :
- When we detect that we are playing a chained off file, the duration is set to infinity,
- The stream is not seekable any more (mSeekable are PR_FALSE, but this doesn't seem to remove the possibility to seek in the controls, so I must be missing something).

Next, I'll see what I can do to allow audio/video ogg chaining, as well as fixing all the aforementioned oddities.
Comment 35 Ralph Giles (:rillian) needinfo me 2011-11-09 11:09:07 PST
Hey Paul. Any update on this? Anything we can do to help?
Comment 36 Paul Adenot (:padenot) 2011-11-09 17:52:11 PST
I've received and set up my new machine (I had a very slow machine for 2 months, and I couldn't work properly), and this is among the first patches I'll rebase and try to land (because of demand for this feature).

Anyway, I've received input from people from the xiph.org that are telling me that it would be great to have audio-only chained-ogg support in Firefox (since audio-only chained-ogg are really used, and only mplayer, as far as I've checked, is the only player to read chained-ogg properly), and since that feature was working in the WIP patch I attached, I am confident that this feature will land soon.
Comment 37 Goran Mekić 2011-11-09 23:31:06 PST
(In reply to Paul ADENOT :padenot from comment #36)
> I've received and set up my new machine (I had a very slow machine for 2

> (since audio-only chained-ogg are really used, and only mplayer, as far as
> I've checked, is the only player to read chained-ogg properly),
No, it's not. Just give it a try: mplayer http://radio.lust4trust.com:8000/fairshare.ogg

On the other hand, mpd works for me like a charm.
Comment 38 Paul Adenot (:padenot) 2011-11-10 02:10:39 PST
Goran, thanks for pointing out my mistake, I was talking about chained theora. AFAIK, the players I've tries (vlc, totem, both on Linux) couldn't handle chained theora, mplayer could, but had problems at stream boundaries. Chrome handles chained vorbis properly, but only plays the first part in a chained theora stream.

Moreover, I just tested the stream you mentioned using mplayer on Ubuntu 11.10, and it handle the chaining pretty well, I can hear multiple songs, and the progress indicator in the console output shows that it is decoding multiple streams (i.e. progress from 0% to 100% multiple times). I'm using MPlayer SVN-r33713-4.6.1.
Comment 39 Goran Mekić 2011-11-10 02:23:05 PST
Mplayer doesn't work on my boxes (4 of them) so I thought it's not working. I guess it about my distro.
Comment 40 Ralph Giles (:rillian) needinfo me 2011-11-10 09:10:10 PST
Thanks for the update, Paul. That's great news.

I agree being able to play Ogg Vorbis streams in Firefox is the highest priority for chaining support. This is something keeping internet radio stations from using HTML5.
Comment 41 Ralph Giles (:rillian) needinfo me 2011-11-25 15:16:57 PST
Any news on this? Anything I can do to help?
Comment 42 Paul Adenot (:padenot) 2011-11-25 15:27:22 PST
I rebased the patch the other day (audio only, for now), I'll attach it tomorrow for review.
Comment 43 Paul Adenot (:padenot) 2011-11-26 09:38:17 PST
Created attachment 577081 [details] [diff] [review]
Part 1 -- Change return value for Decode* methods in nsBuiltinDecoderStateMachine
Comment 44 Paul Adenot (:padenot) 2011-11-26 09:40:08 PST
Created attachment 577082 [details] [diff] [review]
Part 2 -- Implements audio-only chained OGG playback.

This patch passes all the content/media test suite, FWIW.
Comment 45 Paul Adenot (:padenot) 2011-11-28 01:30:47 PST
After quite a few test run, this patch does a regression on test_playback.html, for bug495794.ogg.
Comment 46 Chris Pearce (:cpearce) 2011-11-28 14:18:39 PST
I was looking at this right before you cancelled the review request, and I found the following issues that you should fix before re-requesting review:
* Replaying non-chained media often doesn't work.
* Small links in the chain often aren't played. For example if I play the chained file produced by `cat content/media/test/small-shot.ogg content/media/test/file_a4_tone.ogg`, I don't hear small-shot.ogg.
* Indentation of method arguments in incorrect (in the results-rename patch).

I was testing on Linux.
Comment 47 Paul Adenot (:padenot) 2011-11-28 14:32:30 PST
That is why I've cancelled the review. A way better and reliable implementation is on its way (the idea is there, it needs polishing). The patch as it was was wrong, in fact, but was somehow working in the majority of cases.
Comment 48 Ralph Giles (:rillian) needinfo me 2011-12-16 15:25:12 PST
Any news on the rewrite?
Comment 49 Albert Murillo 2011-12-24 09:16:25 PST
Like Ralph said on #40 This is a big problem that is keeping us [radio broadcasters] from using html5, we're stuck in plugins to play our streams. 

I see Paul is getting close and I hope to see an advance in this, that would be a great but, and its sad to say, underrated improve in order to support and encourage webradios.
Comment 50 Paul Adenot (:padenot) 2012-01-13 03:13:48 PST
I've got a new implementation of that feature , but I run into a problem.
If we play a chain, then |audio_element.duration| should return |Infinity|. At the end of the file, we run into two possible results :

- Either it was a live stream, or a radio broadcast, and the duration is not really relevant. The |duration| property should be |Infinity|.
- Or it was simply a regular chained OGG file with a bounded duration (is it common ?), and then, the duration could be of interest.

As for now, the behavior is to make the stream not infinite (http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoder.cpp#563), to handle the case of a media which was not served with Content-Length (i.e. we don't know the duration before the end of the stream). Consequently, |duration| returns |NaN|, which is incorrect.
Comment 51 David Richards 2012-03-21 02:39:09 PDT
Here is a link to a kludge patch I've cooked up: https://gist.github.com/2145713

It's a kludge because it doesn't attempt to fix the architectural problems, it just attempts to keep playing. It will segfault if you try to seek around after the chain has happened, perhaps this can be covered up by setting Infinite duration, which I don't know how to do. This is my first poke around in firefox proper code, so this is just me poking at the problem with a stick to see what happens.
Comment 52 John Drinkwater (:beta) 2012-06-24 09:37:52 PDT
(In reply to David Richards from comment #51)
> Here is a link to a kludge patch I've cooked up:
> https://gist.github.com/2145713
>

Does this still apply cleanly? would you attach an updated patch here instead of github, so it cant be lost.
Comment 53 Paul Adenot (:padenot) 2012-08-06 18:20:08 PDT
Created attachment 649506 [details] [diff] [review]
Part 1 - Change the return value type of Decode* methods in all backends r=

This patches is rebased and extended from the old patch. It changes the return
values from the Decode* methods from a bool to an enum.
Comment 54 Paul Adenot (:padenot) 2012-08-06 18:29:30 PDT
Created attachment 649510 [details] [diff] [review]
Part 2 - Add support for chained ogg streams, and metadata updating. r=

This patch implements the OGG chaining support. It also send the new metadata
found in the new files to the content (including the user comments).

There is still a bit of work needed. In particular, I'm not sure about the
lifetime of the Tags objects, and what happens when we switch from a stereo
media to a mono media, and I guess this should be handled somehow (either
software downmix or refuse to play the subsequent files, or maybe recreating the
nsAudioStreams).

Also, I'm not too sure if the Vorbis/Opus calls are made properly. I hear
glitches, but it might be because of the way I split the file I use for testing.

content/media tests are green on my machine, but those problems and (probably
more) needs to be addressed.
Comment 55 David Richards 2012-08-21 02:54:10 PDT
Created attachment 653696 [details] [diff] [review]
Firefox Vorbis Chaining Patch

This is an updated version of my earlier kludge patch. This fixes 99% of internet radio chained vorbis streams in the wild.

Differences from original kludge patch:
1) Removal of the word Kludge from the chain state variable
2) Workaround for switching a key in a nsHashTable

What it doesn't do:
1) Opus chaining (This will be easy to add)
2) Sanity checking that this is really more of chained Ogg Vorbis stream with identical audio parameters and not a zipfile of bonzi buddy outfits. It assumes both aux Vorbis headers packets are on a single page which is usually the case.

I'd really like to see this patch with fixes for those two issues make it into Firefox 17.
Comment 56 John Drinkwater (:beta) 2012-08-22 06:58:30 PDT
(In reply to David Richards from comment #55)
> Created attachment 653696 [details] [diff] [review]
> Firefox Vorbis Chaining Patch
> 
> This is an updated version of my earlier kludge patch. This fixes 99% of
> internet radio chained vorbis streams in the wild.

Thanks for the patch, I had issues applying it and build errors from nsnull being undefined, could you have an old HEAD of the tree? afaik nsnull was migrated to nullptr.

> content/media/ogg/nsOggReader.cpp: In member function ‘ogg_packet* nsOggReader::NextOggPacket(nsOggCodecState*)’:
> content/media/ogg/nsOggReader.cpp:790:16: error: ‘nsnull’ was not declared in this scope
Comment 57 John Drinkwater (:beta) 2012-08-22 07:16:32 PDT
(In reply to John Drinkwater (:beta) from comment #56)
> (In reply to David Richards from comment #55)
> > Created attachment 653696 [details] [diff] [review]
> > Firefox Vorbis Chaining Patch
> > 
> > This is an updated version of my earlier kludge patch. This fixes 99% of
> > internet radio chained vorbis streams in the wild.

Can confirm it works for http://radio.randomsonicnet.org:8000/rsnradio
Comment 58 David Richards 2012-08-22 15:09:21 PDT
Thanks for testing, I likely have an old HEAD. I will provide an updated patch sometime tonight that also includes Opus chaining support and sanity checking.
Comment 59 David Richards 2012-08-22 23:07:11 PDT
An update for those following this bug.

I am dropping my candidate out of the race and endorsing Paul's patch. My patch is somewhat of a hit and run, and Paul is more likely to be maintaining code in this area. We worked on the patch for a few hours tonight, and are down to a single assertion failure. There was some backtracing and beardscratching involved, but we are nearly there. I've been told it will have to wait until Firefox 18 as its cutting it to close for 17.

Cheers to Paul!
Comment 60 Paul Adenot (:padenot) 2012-09-14 11:40:50 PDT
Created attachment 661293 [details] [diff] [review]
Part 1 - Change the return value type of Decode* methods in all backends r=

This changes all the Decode* methos on the ns*Reader to make them return:

- FINISHED: when they have finished decoding
- GAP_IN_DATA: when they have no more data, but we should check if there is more
data after the end of the file (i.e. we have a chained file)
- MORE_DATA: when there is more data and we should keep decoding.
- ERROR: when there is an error, and we should stop decoding because the decoder
found

This patch also change the call sites for those functions to reflect the new
interface.
Comment 61 Paul Adenot (:padenot) 2012-09-14 11:44:01 PDT
Created attachment 661295 [details] [diff] [review]
Part 2 - Add support for chained ogg streams, and metadata updating. r=

This implements the actual playback logic for chained files, metadata updates and
such. When we find that a file is chained, we set the file as unseekable to
avoid complication. It works great with chained opus and vorbis files, and does
not affect other files playback.
Comment 62 Paul Adenot (:padenot) 2012-09-14 11:47:18 PDT
Created attachment 661296 [details] [diff] [review]
Part 3 - Test chaining with various files. r=

This patch adds tests, and various chained file (both normal and weird, that is
with changing channel/samplerate accros chains). It is green on try.
Comment 63 Paul Adenot (:padenot) 2012-09-20 23:38:30 PDT
Somehow when rebased against current tip the code that ensures that the new metadata event are dispatched when we play a chain (and not when we decode a new chain) segfaults because nsBuiltinDecoderStateMachine::UpdatePlaybackPosition is called after the audio stream is closed. This was green on try before, but maybe it was an assumption I should not have done. I'll fix that tomorrow.
Comment 64 Paul Adenot (:padenot) 2012-09-24 10:31:11 PDT
Created attachment 664113 [details] [diff] [review]
Part 1 - Change the return value type of Decode* methods in all backends r=

Rebased against tip.
Comment 65 Paul Adenot (:padenot) 2012-09-24 10:32:28 PDT
Created attachment 664115 [details] [diff] [review]
Part 2 - Add support for chained ogg streams, and metadata updating. r=

Rebased against tip, and made the dispatch of the metadata to the main thread
more accurate and safer.
Comment 66 Paul Adenot (:padenot) 2012-09-24 10:34:01 PDT
Created attachment 664116 [details] [diff] [review]
Part 3 - Test chaining with various files. r=

Since we can now read the user comments on opus files, I added tests to check it
works fine with chained ogg that contains opus.
Comment 67 Chris Pearce (:cpearce) 2012-09-25 20:45:43 PDT
I built your patches on top of hg rev 483913c82d2d and I get a test failure in test_chaining.html: "failed | The metadata index value should be incremented. - got "222", expected 0". I'm on Ubuntu 12.04 x64.
Comment 68 Chris Pearce (:cpearce) 2012-09-25 21:14:40 PDT
(In case it wasn't clear, I was running the tests locally when I saw the failure in test_chaining).

Some streaming sites provided by oneman for testing:
http://dubsteplight.moeradio.ru:23000/dubsteplight.ogg
http://listen.42fm.ru:8000/stealkill-1.0.ogg

While playing these I keep seeing warnings spamming my console:

WARNING: Can't add a range if the end is older that the start.: file /home/cpearce/src/mozilla/yellow/content/html/content/src/nsTimeRanges.cpp, line 61

I see these on other files too. Can you prevent this warning?

I also playback of those streams stalling occasionally. I suspect this is us buffering; it would be good if we could show the throbber in such cases.
Comment 69 Chris Pearce (:cpearce) 2012-09-30 16:17:23 PDT
Comment on attachment 664115 [details] [diff] [review]
Part 2 - Add support for chained ogg streams, and metadata updating. r=

Review of attachment 664115 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this.

I'd like to reduce the impact on non-Ogg backends of this patch by restricting how we support Ogg chaining. That will make our code simpler and cleaner.

The solution I previously described, and what you implemented here, was designed to handle video in chains, but since we decided to limit our chaining support to audio only we can make make this much simpler.

::: content/media/nsBuiltinDecoder.cpp
@@ -547,2 @@
>  {
> -  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");

Why did you remove this check? Seems like we should have it.

@@ -570,2 @@
>  
> -  if (!mResourceLoaded) {

mmm why are you removing this code? We should still be calling StartProgress() and dispatching "progress" shouldn't we?

::: content/media/nsBuiltinDecoder.h
@@ +652,5 @@
>    // Call on the decode thread only.
>    virtual void OnReadMetadataCompleted() { }
>  
> +  // Called when we need to report new metadata, during playback (because we are
> +  // playgin a new media in a chained media).

s/playgin/playing/

@@ +655,5 @@
> +  // Called when we need to report new metadata, during playback (because we are
> +  // playgin a new media in a chained media).
> +  void UpdateMetadata(uint32_t aChannels,
> +                      uint32_t aRate,
> +                      bool aHasaudio,

aHasAudio

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +914,5 @@
> +        case nsBuiltinDecoderReader::FINISHED: {
> +          audioPlaying = false;
> +          break;
> +        }
> +        case nsBuiltinDecoderReader::GAP_IN_DATA: {

It seems like we only need this GAP_IN_DATA return value to handle the case where we have a audio+video file with subsequent chained audio, and we need to decode across the remainder of the first video link.

If we instead allow chaining *only* in audio-only chains, and just fail if we detect a gap, then we don't need to change the return values of the Decode*Data functions, and the code becomes much cleaner.

So I think we should only allow chaining in resources that:
* Have only 1 audio stream (Vorbis or Opus) in every link and no video stream present in any link.
* Have subsequent links in the chain streams starting in the page *immediately* after the e_o_s page of the preceding link.

Then in nsOggReader::DecodeAudioData() when you encounter an e_o_s page you check to see if you've only ever had 1 audio stream present in every link so far, and if so you try to read metadata from the next link, and if you don't find one immediately in the next page you give up and return end of stream. If you find another link then add its samples to the audio queue.

(Future work could be to down-mix audio samples from links whose bitrate doesn't match the bitrate of the first link).

When nsOggReader::DecodeAudioData encounters another link propagate the new metadata to the MediaMetadataManager (see below), and the decoder state machine polls the MediaMetadataManager in AdvanceFrame() to see if its time for the media element to dispatch a new metadata event.

That should reduce the amount of code needed to support chaining, and reduce the impact of supporting Ogg chaining on non-Ogg backends and the state machine.

@@ +918,5 @@
> +        case nsBuiltinDecoderReader::GAP_IN_DATA: {
> +          // Try to read metadata. If we succeed, we are playing a chained
> +          // media. If it fails, we reached the end of the media.
> +          nsVideoInfo info;
> +          nsHTMLMediaElement::MetadataTags* tags = new nsHTMLMediaElement::MetadataTags;

I think we should encapsulate all of the code related to metadata into its own class (MediaMetadataManager perhaps) in its own file if possible.

My goal here is to isolate the logical parts of the code so that each part does less, and thus is easier to understand. Currently the decoder state machine doesn't do that very well, I'd like to improve things here.

@@ +1353,5 @@
>  
> +  // Check if it's time to send the new metadata we got to the content.
> +  if (!mMetadataQueue.empty() && mAudioStream) {
> +    if(mMetadataQueue.front().mTime < mAudioStream->GetPosition()) {
> +      printf("dispatching: mTime: %ld, time: %ld\n", mMetadataQueue.front().mTime, mAudioStream->GetPosition());

Don't add naked printfs. Use our log macros.

You should create a method on your new MediaMetadataManager object to dispatch new metadata loaded events if necessary here.

@@ +1838,5 @@
>    }
>  
>    NS_ASSERTION(mStartTime != -1, "Must have start time");
>    NS_ASSERTION((!HasVideo() && !HasAudio()) ||
> +                !mSeekable || mEndTime != -1 || IsChained(),

It would be good if you can get things to a state where we don't need the IsChained() resource; i.e. if we can somehow treat the chained resource as not seekable, rather than needing to special case on IsChained(). We should know right from the start that a chained Ogg resource isn't seekable, even though we might not know that it's chained until playback reaches the second link, right?

@@ +2173,5 @@
>  
>      case DECODER_STATE_COMPLETED: {
>        StopDecodeThread();
>  
> +      //if (mState != DECODER_STATE_COMPLETED) {

Don't add commented out code.

Hopefully you won't need this case once you make the comments I suggested above; changing this makes me nervous, we've had a lot of bugs which have needed to be fixed by special cases like this, so removing those special cases makes me worry we'll regress something.
Comment 70 Chris Pearce (:cpearce) 2012-09-30 16:18:25 PDT
Comment on attachment 664113 [details] [diff] [review]
Part 1 - Change the return value type of Decode* methods in all backends r=

Review of attachment 664113 [details] [diff] [review]:
-----------------------------------------------------------------

Hopefully we won't need this patch once my comments in the other patch are addressed. You can re-request review if we end up needing this.
Comment 71 David Richards 2012-10-05 05:08:09 PDT
Created attachment 668401 [details] [diff] [review]
Minimalist Ogg Chaining Fix

This patch is essentially a bulletproofed version of my original kludge patch. It's as minimal and bulletproof as I can come up with, it only touches the nsOggReader class and does not operate within any other threads, so it should be regression safe.

The one missing piece is to disable seeking upon discovery of the chain, I am uncertain how this should be done, but have left a placeholder for it. Even with seeking not disabled I have not been able to make it crash.

I'd like to hold back on patches for metadata, video chaining, codec switching, and all other manner of fancy funstuff until we can get to an agreement on this very basic and critical matter that has bugged us all so much for four years.
Comment 72 David Richards 2012-10-06 06:14:52 PDT
I would like to amend my comment above about missing piece and seeking, I do not mean there is a missing piece to this patch, I mean that there is not a method to disable seeking at the container level. The seeking I speak of is seeking at the transport level. This patch is complete for purposes of fixing this problem without regression.
Comment 73 David Richards 2012-10-06 06:59:03 PDT
If we do not add the IsChained() method, there would need to be a way for the decoding thread to set mSeekable to false. Until seeking within chained ogg is actually supported, code for which I will be delighted to write, it will be a special case scenario.
Comment 74 Chris Pearce (:cpearce) 2012-10-14 21:35:40 PDT
Comment on attachment 668401 [details] [diff] [review]
Minimalist Ogg Chaining Fix

Review of attachment 668401 [details] [diff] [review]:
-----------------------------------------------------------------

This is close to what I suggested in my comments on Paul's last patch. I don't think this patch needs a lot more work to get it land-able, so maybe you could keep working on it?

I've put a simple testcase that plays one of the streams that you found here:
http://people.mozilla.org/~cpearce/chain.html
This shows the buffered, seekable and played ranges, and the "loadedmetadata" events and enumerates the metadata as it changes. The buffered and seekable ranges should be empty after the first link finishes playing, currently they're sometimes non-zero after the first chain, which is at best misleading.

::: content/media/ogg/nsOggReader.cpp
@@ +600,5 @@
> +  bool chained = false;
> +  nsOpusState* newOpusState = nullptr;
> +  nsVorbisState* newVorbisState = nullptr;
> +
> +  if (HasVideo() || HasSkeleton() || !HasAudio()) {

It would be nice to support chaining if there's a skeleton in the links, but for now it's OK to just fail if it means we get to shipping this.

@@ +630,5 @@
> +    newOpusState = static_cast<nsOpusState*>(codecState);
> +  }
> +#endif
> +
> +  mCodecStates.Put(serial, codecState);

mCodecStates and mKnownStreams are accessed in GetBuffered() which is called from multiple threads. In order for this to be safe, they're not supposed to change after metadata is read. You're changing them here, so you'll need to add a monitor to protect access to these fields, and any other fields that both ReadChain() and GetBuffered() accesses.

You should require the monitor to be held for *reads* on which aren't on the decode thread, and writes which happen on the decode thread. You don't need to worry about reads on the decode thread.

The only function which reads these fields off the decode threads is GetBuffered(). You'll need to hold the monitor for most if not all of GetBuffered().

@@ +642,5 @@
> +
> +  if ((newVorbisState && ReadHeaders(newVorbisState)) &&
> +      (mVorbisState->mInfo.rate == newVorbisState->mInfo.rate) &&
> +      (mVorbisState->mInfo.channels == newVorbisState->mInfo.channels)) {
> +    mVorbisState->Reset();

You should also remove mVorbisState from mCodecStates and its serial from mKnownStreams. Ditto for the mOpusState. Otherwise their memory won't be released until the audio element is shutdown.

Once you do this, it'll be doubly important that you hold the monitor in this function, otherwise GetBuffered() could be accessing these codecStates while they're being destroyed.

@@ +661,5 @@
> +  }
> +#endif
> +
> +  if (chained) {
> +    //Set seeking to false here

How about setting a flag here mIsChained, and have Seek() and GetBuffered() fail if mIsChained is true. You'll need to protect reads and writes to mIsChained with a monitor (you can use the same monitor you use to protect mCodecStates and mKnownStates), since GetBuffered() is called on the main and state machine threads.

Also note that if GetBuffered() doesn't return anything the logic to tell if the download is keeping up with the decode won't work. I have plans to rewrite how GetBuffered() works which will incidentally alleviate this, but until I get to that, we can probably live with this.

You also needs to ensure here that a new loadedmetadata gets dispatched to the media element, and a new nsHTMLMediaElement::MetadataTags gets sent to the media element. You can probably just dispatch a new nsAudioMetadataEventRunner to the decoder; you'll need to expose nsAudioMetadataEventRunner somehow (it's in nsBuiltinDecoderStateMachine.cpp). That'll be easier than refactoring the the metadata handling code out into its own class.
Comment 75 Paul Adenot (:padenot) 2012-11-26 05:13:59 PST
Created attachment 685113 [details] [diff] [review]
Add support for chained ogg audio file and proper metadata. r=

Here is a new version, implementing the design discussed in previous message,
and back-porting all the metadata handling and synchronization stuff.

There is a thing I don't like, though: |MediaDecoderStateMachine::SetSeekable|
is weird, but this is is kind of required if we don't want to change the API of
the *Reader, to add an |IsChained()| method.

Indeed, at the decoding level, we know that we should disable seeking because we
are playing a chained media, but right after having called |SetSeekable(false)|
on the decoder from the reader, the second HTTP request (that we do to find the
duration, when we are reading an OGG media) calls back, and sets the media as
seekable, because the range request suceeded.

Maybe it would be better for the reader to call back into the decoder, setting
a flag or something.

Also, green on try: https://tbpl.mozilla.org/?tree=Try&rev=a129afd2639a
Comment 76 Paul Adenot (:padenot) 2012-11-26 05:16:25 PST
Created attachment 685114 [details] [diff] [review]
Tests for ogg chain support. r=

Nothing too different. I relaxed the metadata index check because of
very rare orange (probably gc kicking in a delaying the event loop or something
along these lines).

Also, I fixed the test files (there was a index=222 in one of them for some
reason).
Comment 77 Chris Pearce (:cpearce) 2012-11-26 13:05:13 PST
Comment on attachment 685114 [details] [diff] [review]
Tests for ogg chain support. r=

Review of attachment 685114 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/manifest.js
@@ +330,5 @@
>  var gFragmentTests = [
>    { name:"big.wav", type:"audio/x-wav", duration:9.278981, size:102444 }
>  ];
>  
> +// Used by test_chaining.html. The |parts| attributes is the number of parts in

s/part/link/g

That's the correct terminology WRT Ogg chain segments. The next "link" in the "chain".

@@ +347,5 @@
> +  { name:"variable-samplerate.ogg", type:"audio/ogg", parts: 1 },
> +  // Opus decoding in Firefox outputs 48 kHz PCM despite having a different
> +  // original sample rate, so we can safely play Opus chained media that have
> +  // different samplerate accross parts.
> +  { name:"variable-samplerate.opus", type:"audio/ogg; codec=opus", parts: 2 },

Also include a chained video file, we should not play subsequent links in the chain on video files, so they should only claim to have 1 part for your test to still pass I guess.

::: content/media/test/test_chaining.html
@@ +51,5 @@
> +  if (! t._metadataCount) {
> +    t._metadataCount = 0;
> +  }
> +
> +  // We should be able to assert equality here, but somehow it fails (rarely but

Probably because of the async way we dispatch metadata to the media element.

@@ +60,5 @@
> +
> +  // The files have all a user comment name 'index' that increments at each link
> +  // in the chained media.
> +  t._metadataCount++;
> +  t.play();

Just give the media an "autoplay" attribute, otherwise if the loadedmetadata event arrives after the element has finished replaying, you'll restart playing the media, which may mess up your test.
Comment 78 Chris Pearce (:cpearce) 2012-11-26 17:19:57 PST
(In reply to Paul ADENOT (:padenot) from comment #75)
> Created attachment 685113 [details] [diff] [review]
> Add support for chained ogg audio file and proper metadata. r=
> 
> Here is a new version, implementing the design discussed in previous message,
> and back-porting all the metadata handling and synchronization stuff.
> 
> There is a thing I don't like, though:
> |MediaDecoderStateMachine::SetSeekable|
> is weird, but this is is kind of required if we don't want to change the API
> of
> the *Reader, to add an |IsChained()| method.
> 
> Indeed, at the decoding level, we know that we should disable seeking
> because we
> are playing a chained media, but right after having called
> |SetSeekable(false)|
> on the decoder from the reader, the second HTTP request (that we do to find
> the
> duration, when we are reading an OGG media) calls back, and sets the media as
> seekable, because the range request suceeded.
> 
> Maybe it would be better for the reader to call back into the decoder,
> setting
> a flag or something.

Is the problem that we've overloaded what it means to be "seekable"? We use it to mean two things: the underlying HTTP channel supports byte range requests, and the resource supports random access.

Please separate these two meanings into distinct flags called IsTransportSeekable(), which means the MediaResource is seekable (supports HTTP byte ranges or is a local file) and IsSeekingSupported() which means that the video or audio file is able to seek, if random access into the media is possible.

IsSeekingSupported() should return false when playing a chained media, or when playing a WebM video which doesn't have an index.

We then can actually only seek if IsSeekingSupported() && IsSeekingSupported().

If that doesn't make sense or won't work, tell me why and suggest something better.
Comment 79 Chris Pearce (:cpearce) 2012-11-26 17:21:46 PST
Comment on attachment 685113 [details] [diff] [review]
Add support for chained ogg audio file and proper metadata. r=

Review of attachment 685113 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there. :)

::: content/html/content/public/nsHTMLMediaElement.h
@@ +119,5 @@
>  
>    // Called by the video decoder object, on the main thread,
>    // when it has read the metadata containing video dimensions,
>    // etc.
> +  virtual void MetadataLoaded(mozilla::TimedMetadata* aMetadata) MOZ_FINAL MOZ_OVERRIDE;

I'd actually prefer it if we didn't pass TimedMetadata* out to nsHTMLMediaElement, as TimedMetadata exposes a timestamp in microseconds, which nsHTMLMediaElement doesn't and shouldn't need.

Instead, have MediaMetadataManager store TimedMetadata objects (like you're doing), but have MediaMetadataManager dispatch the metadata using the existing AudioMetadataEventRunner to call nsHTMLMediaElement::MetadataLoaded(channels, rate, hasAudio, tags). i.e. don't expose the TimedMetadata wrapper to nsHTMLMediaElement, only use it inside content/media.

Make sense?

::: content/media/MediaDecoderStateMachine.cpp
@@ +1382,1 @@
>  

Leave the assertion that we're in the decoder monitor.

@@ +1790,2 @@
>    nsCOMPtr<nsIRunnable> metadataLoadedEvent =
> +    new AudioMetadataEventRunner(mDecoder, metadata);

Keep the original code, i.e. don't wrap the params in TimedMetadata. This event runs ASAP, it doesn't need a TimedMetadata::m*Time.

@@ +2162,5 @@
>    if (aData->mDuplicate) {
>      return;
>    }
>  
> +  if (!PR_GetEnv("MOZ_QUIET")) {

We should actually make this change (and the other logging changes) in a separate patch, as it's logically not part of a chaining implementation. What crazy fool suggested you do this. ;)

@@ +2694,5 @@
> +void MediaDecoderStateMachine::QueueMetadata(TimedMetadata* aMetadata)
> +{
> +  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
> +  mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> +  mMetadataManager.QueueMetadata(aMetadata);

Construct the TimedMetadata here.

::: content/media/MediaDecoderStateMachine.h
@@ +322,5 @@
>    // Returns true if the state machine has shutdown or is in the process of
>    // shutting down. The decoder monitor must be held while calling this.
>    bool IsShutdown();
>  
> +  void QueueMetadata(TimedMetadata* aMetadata);

Pass in raw arguments, then you don't have to write code to construct TimedMetadata every where you call this, you only need to do it once inside this function.

::: content/media/MediaMetadataManager.h
@@ +11,5 @@
> +#include "AbstractMediaDecoder.h"
> +
> +namespace mozilla {
> +
> +  typedef nsDataHashtable<nsCStringHashKey, nsCString> MetadataTags;

Don't indent code inside namespaces.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Namespaces

@@ +19,5 @@
> +    private:
> +      nsRefPtr<AbstractMediaDecoder> mDecoder;
> +    public:
> +      AudioMetadataEventRunner(AbstractMediaDecoder* aDecoder, TimedMetadata* aMetadata)
> +      : mDecoder(aDecoder),

These lines beneath the constructor declaration should be indented one tab.

@@ +29,5 @@
> +      mDecoder->MetadataLoaded(mMetadata);
> +      return NS_OK;
> +    }
> +
> +      TimedMetadata* mMetadata;

Indentation is wrong here.

@@ +37,5 @@
> +  // metadata should start to be reported.
> +  class TimedMetadata : public LinkedListElement<TimedMetadata> {
> +    public:
> +      // The time, in microseconds, at which those metadata should be available.
> +      int64_t mTime;

Call this mPublishTime. We have lots of things called "time" in the media code, it's good to disambiguate.

"publish" is better than the other obvious candidate "available", since "available" could be interpreted as the mean the duration of time which it's available *for*, which is not what this field is used for.

@@ +38,5 @@
> +  class TimedMetadata : public LinkedListElement<TimedMetadata> {
> +    public:
> +      // The time, in microseconds, at which those metadata should be available.
> +      int64_t mTime;
> +      // The metadata.

Comment should describe life cycle; who deletes this object.

Perhaps you should make this an AutoPtr, and forget() it when you dispatch it.

@@ +56,5 @@
> +    public:
> +      ~MediaMetadataManager() {
> +        TimedMetadata* element;
> +        while((element = mMetadataQueue.popFirst()) != nullptr) {
> +          delete element;

If you don't make TimedMetadata::mTags an AutoPtr, you need to delete mTags here right?

@@ +64,5 @@
> +        mMetadataQueue.insertBack(aMetadata);
> +      }
> +
> +      void DispatchMetadataIfNeeded(AbstractMediaDecoder* aDecoder, double aCurrentTime) {
> +        TimedMetadata* metadata = mMetadataQueue.getFirst();

I think you should do this in a loop. "while publish time is less than current time, dispatch". Otherwise if you have a very short duration link, the metadata may fall behind.

::: content/media/ogg/OggReader.cpp
@@ +404,5 @@
>                                     frames,
>                                     buffer.forget(),
>                                     channels));
> +
> +    mLastTimeDecoded += duration;

You'll get gradual rounding errors over time when you accumulate this, even with units as small as microseconds. Store the time as a sum of audio samples instead, then you won't get rounding errors.

@@ +640,5 @@
> +  if (mCodecStates.Get(serial, nullptr)) {
> +    return false;
> +  }
> +
> +  OggCodecState* codecState = 0;

Make this an AutoPtr<OggCodecState>, so that if we exit on a failure path without inserting into mCodecStates, the codec state gets released.

@@ +653,5 @@
> +#ifdef MOZ_OPUS
> +  if (mOpusState && (codecState->GetType() == OggCodecState::TYPE_OPUS)) {
> +    newOpusState = static_cast<OpusState*>(codecState);
> +  }
> +#endif

You should return false here if we don't have an opus or a vorbis stream, as you might have a video stream, and we don't want to play that.

Making this block "if (vorbis) {...} else if (opus) {...} else {return false;}" etc would probably suffice.

You should have a testcase for this case (audio stream followed by video stream).

@@ +655,5 @@
> +    newOpusState = static_cast<OpusState*>(codecState);
> +  }
> +#endif
> +
> +  mCodecStates.Put(serial, codecState);

codecState.forget() once it's an AutoPtr...

::: content/media/ogg/OggReader.h
@@ +50,5 @@
>                                  MetadataTags** aTags);
>    virtual nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime);
>    virtual nsresult GetBuffered(nsTimeRanges* aBuffered, int64_t aStartTime);
>  
> +  // We use bisection to seek in buffered range, bug we dont allow seeking in a

s/bug we dont/but we don't/

@@ +58,5 @@
>    }
>  
>  private:
>  
> +  ReentrantMonitor mMonitor;

Comment explaining what mMonitor protects.

@@ +221,5 @@
>  
> +  // Reads the next link in the chain.
> +  bool ReadOggChain();
> +
> +  // Set this media as being and chain and notifies the state machine that the

s/Set this media as being and chain/Set this media as being a chain/
Comment 80 Paul Adenot (:padenot) 2012-11-29 07:07:41 PST
Created attachment 686581 [details] [diff] [review]
Put some very verbose media logging behind a MOZ_QUIET=1 check. r=

This is split out from the first patch.
Comment 81 Paul Adenot (:padenot) 2012-11-29 07:29:21 PST
Created attachment 686587 [details] [diff] [review]
Review comments. Should be folded before landing.

This is the changes on top of the r- patch, it might be easier for you to
review. In any case, I'll upload the whole folded patch in a minute.

Regarding comment 78, I've settled on "SetMediaSeekable" and
"SetTransportSeekable" and the associated "Is{Tranport,Media}Seekable" and
checked and modified all the call sites. I've also added a couple comments to
tell the difference between the two set of methods.

And also all the other comments have been addressed. Still shows a beautiful
green on try: https://tbpl.mozilla.org/?tree=Try&rev=5cc5b881d302
Comment 82 Paul Adenot (:padenot) 2012-11-29 07:30:49 PST
Created attachment 686588 [details] [diff] [review]
Tests for ogg chain support. r=

Comments addressed + added testcase that has 4 links on audio only, then a link
of video, as noted on comment 79.
Comment 83 Paul Adenot (:padenot) 2012-11-29 07:33:05 PST
Created attachment 686590 [details] [diff] [review]
Add support for chained ogg audio file and proper metadata dispatching. r=

Folded up patch, feel free to review this one instead if you prefer.
Comment 84 Chris Pearce (:cpearce) 2012-11-29 11:28:22 PST
Comment on attachment 686581 [details] [diff] [review]
Put some very verbose media logging behind a MOZ_QUIET=1 check. r=

Review of attachment 686581 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaDecoderStateMachine.h
@@ +82,5 @@
>  #include "mozilla/ReentrantMonitor.h"
>  #include "nsITimer.h"
>  #include "AudioSegment.h"
>  #include "VideoSegment.h"
> +#include "prenv.h"

Put this in the cpp file; it will mean it's not (unnecessarily) included in everything that includes MediaDecoderStateMachine.h, and thus reduce compile time.
Comment 85 Chris Pearce (:cpearce) 2012-11-29 16:02:00 PST
Comment on attachment 686587 [details] [diff] [review]
Review comments. Should be folded before landing.

Review of attachment 686587 [details] [diff] [review]:
-----------------------------------------------------------------

Bravo!

r=cpearce with comments below addressed.

::: content/media/AbstractMediaDecoder.h
@@ +72,1 @@
>    // Set the media as being seekable or not.

These comments need to be swapped; you have the comment for SetTransportSeekable() on SetMediaSeekable() and vice versa.

::: content/media/MediaDecoder.cpp
@@ +1229,3 @@
>  }
>  
>  nsresult MediaDecoder::GetSeekable(nsTimeRanges* aSeekable)

Won't this return buffered ranges even if !(IsTransportSeekable() && IsMediaSeekable()) && IsSeekableInBufferedRanges()? i.e. if IsSeekableInBufferedRanges() returns true we'll return ranges even if the media isn't seekable?

I think GetSeekable should be:

if (!IsMediaSeekable())
  // return no ranges
else if (!IsTransportSeekable())
  if IsSeekableInBufferedRanges()
    // return buffered ranges
  else
    // return no ranges
else
  return [0...end]  (or 0...inf)

I don't think we need IsSeekableInBufferedRanges() anymore either? Please also file a bug to remove IsSeekableInBufferedRanges() and have the classes that return false for it call SetIsMediaSeekable(false) instead.

::: content/media/MediaDecoder.h
@@ +507,5 @@
> +  virtual void SetMediaSeekable(bool aMediaSeekable) MOZ_FINAL MOZ_OVERRIDE;
> +  virtual void SetTransportSeekable(bool aTransportSeekable) MOZ_FINAL MOZ_OVERRIDE;
> +  // Returns true if this media supports seeking. False for example for WebM
> +  // files without an index and chained ogg files.
> +  virtual bool IsMediaSeekable() MOZ_FINAL MOZ_OVERRIDE;

Can you file a follow up bug to have the WebM decoder set the media as unseekable if it doesn't have an index? Thanks. You could do this in the bug to remove IsSeekableInBufferedRanges even.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1477,5 @@
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>  
> +  // We need to be able to seek both at a transport level and at a media level
> +  // to seek.
> +  if (!(mTransportSeekable || mMediaSeekable)) {

Not true, we can still seek in buffered ranges if !mTransportSeekable && mMediaSeekable; so remove the !mTransportSeekable condition here. Right?

::: content/media/MediaDecoderStateMachine.h
@@ +180,5 @@
>    // be called with the decode monitor held.
>    void ClearPositionChangeFlag();
>  
>    // Called from the main thread to set whether the media resource can
>    // seek into unbuffered ranges. The decoder monitor must be obtained

Comment is out of date now.

@@ +185,5 @@
>    // before calling this.
> +  void SetTransportSeekable(bool aSeekable);
> +
> +  // Called from the main thread to set whether the media can seek into buffered
> +  // ranges. This is not true for chained ogg and WebM media without index. The

s/seek into buffered ranges/seek to random locations/.

We can seek into any location, even unbuffered one, if the transport layer supports it.

::: content/media/ogg/OggReader.cpp
@@ +640,5 @@
>    if (mCodecStates.Get(serial, nullptr)) {
>      return false;
>    }
>  
> +  nsAutoPtr<OggCodecState> codecState(nullptr);

You don't need the (nullptr) here.
Comment 88 Ed Morley [:emorley] 2012-12-06 15:52:20 PST
Please make sure the target milestone is correct when landing. Multiple groups treat it as an absolute source of truth (a.m.o, MDN etc), so it's imperative it is correct.

If the field is empty, m-cMerge will set it for you when merging, however in this case it had previously been set, so it needed to be correct.
Comment 89 Antoine Turmel [:GeekShadow] 2012-12-13 06:45:28 PST
Is it possible to merge this in mozilla18 for Firefox OS ?
Comment 90 Paul Adenot (:padenot) 2012-12-13 10:13:55 PST
Antoine, if you feel this is important enough and has enough use case, you can try to ask for it.
Comment 91 Antoine Turmel [:GeekShadow] 2012-12-17 01:51:56 PST
Important enough : maybe not
Use case : playing web tv/radios in webapps
Comment 92 Paul Adenot (:padenot) 2012-12-17 02:22:03 PST
This patch only implements playback of chained ogg audio-only files.
Comment 93 Anand Kumria 2012-12-22 21:32:55 PST
So the use-case of: playing a web-based radio station on your Firefox OS phone is still viable.

Who needs to be pinged to have this uplifted into Firefox OS?
Comment 94 Thomas B. Rücker [account no longer active] 2012-12-23 00:34:19 PST
(In reply to Anand Kumria from comment #93)
> So the use-case of: playing a web-based radio station on your Firefox OS
> phone is still viable.
> 
> Who needs to be pinged to have this uplifted into Firefox OS?

I support this view. From a previous discussion with Paul I understand that we have to ask for "blocking-basecamp+" to be added to this bug. Sadly I'm not active enough recently to know who needs to be poked for this to happen and how to do this.

The reasoning behind this is simple:
Internet radio is a popular thing nowadays, as is listening to music from your mobile phone. In addition the Opus codec allows us to do this in a bandwidth efficient manner. This works with Icecast 2.4 (currently in beta with production grade Opus support).
Where mobile data connections are too expensive, WiFi remains as a solid transport available in most places nowadays.

Also let me take this opportunity to thank everyone involved in seeing this bug through to a landed fix. As an Icecast maintainer it has been an aching thorn in my side for very long.
Comment 95 Robert Kaiser 2012-12-27 05:04:45 PST
(In reply to Thomas B. Rücker from comment #94)
> I support this view. From a previous discussion with Paul I understand that
> we have to ask for "blocking-basecamp+" to be added to this bug. Sadly I'm
> not active enough recently to know who needs to be poked for this to happen
> and how to do this.

The right process for that is to nominate for that flag by setting it to "?". I have done that now, the team will go through all nominations and decide to + or - based on importance and time/risk analysis.
Comment 96 Andrew Overholt [:overholt] 2012-12-27 10:11:02 PST
We discussed this during triage today.  This is important but it isn't something we'd hold the release of the phone for (bb+).  Please just ask for uplift (approval‑mozilla‑b2g18, etc.) on the patches in situations like this.  I would have done so here but I don't see what patch should be uplifted.
Comment 97 Paul Adenot (:padenot) 2013-02-08 06:37:05 PST
Comment on attachment 686590 [details] [diff] [review]
Add support for chained ogg audio file and proper metadata dispatching. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Chained OGG (opus, vorbis) support
User impact if declined: The user cannot play most web radios
Testing completed: Baked for a super long time on central, with unit tests
Risk to taking this patch (and alternatives if risky): Firefox OS user won't be able to listen to radio shows
String or UUID changes made by this patch: None.

I keep being asked if this feature will be available on Firefox OS V1, and I have to say that unfortunately, it did not make it in time.
Comment 98 Alex Keybl [:akeybl] 2013-02-08 16:02:47 PST
Comment on attachment 686590 [details] [diff] [review]
Add support for chained ogg audio file and proper metadata dispatching. r=

Sorry, this is not within the feature set of v1.x and is therefore unnecessary risk at this point.
Comment 99 Evan Carroll 2013-02-12 11:18:25 PST
I just wanted to chime in. This isn't fixed. I have a radio station up right now:

http://radio.evancarroll.com:8000/stream.opus

Firefox will never play across opus-boundaries in the stream. After whatever song is playing concludes the stream terminates.
Comment 100 Ralph Giles (:rillian) needinfo me 2013-02-12 12:07:20 PST
(In reply to Evan Carroll from comment #99)
> I just wanted to chime in. This isn't fixed.

What version of Firefox were you testing with? This bug is fixed for Firefox 20 or later. See the 'Target Milestone' field at the top of the page. This is currently Firefox Aurora and will become part of the official Firefox stable release in April.

The stream does play across song boundaries for me in Firefox 21 (current nightly).
Comment 101 Evan Carroll 2013-02-12 13:16:06 PST
Firefox 18.0.2. It may have been fixed at a later point in time. I'll check out 19 when it rolls out to the ppas next week.
Comment 102 Paul Adenot (:padenot) 2013-03-11 05:33:39 PDT
*** Bug 494504 has been marked as a duplicate of this bug. ***
Comment 103 Emilis Dambauskas 2013-06-06 09:20:34 PDT
What are the plans for fixing this bug on Firefox OS?

I am developing an Internet radio player for Firefox OS (<https://github.com/emilis/worldradioplayer>) and I need this fix.

I am currently using an ugly workaround in JavaScript that reloads the stream if it ends on itself. The workaround skips a couple seconds of playback and probably introduces some problems in other cases.

The code for the workaround is here:
https://github.com/emilis/worldradioplayer/blob/master/static/js/sound-player.js#L86

Anyway, fixing this bug in JavaScript seems like the wrong thing to do, as it would require complicated code to catch the exact state when it happens without having the full information. So I am hoping for a fix to arrive someday sooner.
Comment 104 Paul Adenot (:padenot) 2013-06-06 09:55:00 PDT
Emilis, the fix will arrive in Firefox OS as soon as Firefox OS decides to update its version of Gecko so it is not based on 18. Not sure about a specific timeline.

As you can see, people have asked for approval to uplift this patch to b2g, but it got rejected.
Comment 105 Sudheesh Singanamalla (:ShellHacker) 2013-07-06 02:15:35 PDT
*** Bug 738990 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.