Last Comment Bug 422538 - Ogg Theora backend for HTML5 video element
: Ogg Theora backend for HTML5 video element
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: P1 enhancement with 15 votes (vote)
: mozilla1.9.1a2
Assigned To: cajbir (:cajbir)
:
Mentors:
Depends on: video 447639 448534 448651
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-12 16:13 PDT by cajbir (:cajbir)
Modified: 2010-01-22 01:32 PST (History)
41 users (show)
jonas: wanted1.9.1+
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to add theora decoder to html video element support (141.58 KB, patch)
2008-03-12 21:34 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Version 2 of the Theora/Vorbis video/audio backend (132.63 KB, patch)
2008-07-03 16:00 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds libogg-1.1.3 to modules subdirectory (175.18 KB, patch)
2008-07-23 10:32 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds libvorbis-1.2.0 to modules subdirectory (550.87 KB, patch)
2008-07-23 10:35 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds libtheora-1.0beta3 to modules subdirectory (416.07 KB, patch)
2008-07-23 10:37 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds liboggz-0.9.8 to modules subdirectory (367.92 KB, patch)
2008-07-23 10:39 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds libfishsound-0.9.1 to modules subdirectory (201.87 KB, patch)
2008-07-23 10:41 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Adds liboggplay to modules subdirectory (287.32 KB, patch)
2008-07-23 10:44 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Version 3 of the Theora/Vorbis video/audio backend (76.31 KB, patch)
2008-07-23 10:50 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Version 4 of the Theora/Vorbis video/audio backend (81.99 KB, patch)
2008-07-29 17:50 PDT, cajbir (:cajbir)
roc: review+
roc: superreview+
Details | Diff | Review

Description cajbir (:cajbir) 2008-03-12 16:13:11 PDT
Bug 382267 is being restructured to not contain implementations of decoders for video codecs. The Ogg Theora support from that bug will be moved here.
Comment 1 cajbir (:cajbir) 2008-03-12 21:34:20 PDT
Created attachment 309032 [details] [diff] [review]
Patch to add theora decoder to html video element support

This is the theora code that was originally in bug 382267. Apply the patch from 382267 to get video support, then this patch. You'll also need the theora libraries. This is available as a patch here (It's a bit big to attach):

http://www.double.co.nz/video_test/modules1.patch.gz

To build with it included use --enable-ogg in .mozconfig.
Comment 2 Fabien Tassin 2008-03-13 06:24:07 PDT
Most (if not all) linux distro already have those libs.
It would be nice to have the corresponding --with-system-xxx=/path supported in configure. For example, Debian & Ubuntu already have vorbis, ogg, oggz, theora and fishsound. Only ogg_play would be needed (until it is packaged).

BTW, will it coexist with GStreamer from bug 422540? I mean, you will have more than one decoder for a given format. Which will take preference?
Comment 3 cajbir (:cajbir) 2008-03-13 14:49:48 PDT
Yes, good point about the configure settings for system libs. I'll look at it. 

It coexists in that both patches can be applied with minimal conflicts. Then nsHTMLVideoElement.cpp needs to be modified to pick the preference (probably based on mime type) for instantiating the decoder. I guess it would be better to have some way of asking the decoder what types they handle and instantiating based on that. But what if they both handle the same mime type?
Comment 4 Henri Sivonen (:hsivonen) 2008-03-14 02:15:27 PDT
>I guess it would be better to have some way of asking the decoder what types they handle and instantiating based on that.

Indeed, but can GStreamer, QuickTime and DirectShow codecs report what RFC 4281 codec values they support?

It would be good to get RFC 4281 implementability feedback to the HTML WG/WHATWG.
Comment 5 Damon Sicore (:damons) 2008-06-04 16:45:34 PDT
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Comment 6 Ryan A. C. 2008-06-19 21:15:38 PDT
(In reply to comment #3)
> But what if they both handle the same mime type?

The system codec should probably take precedence over the internal codec in that case. On windows at least there are a number of directshow filters that people like to apply for color correction, subtitle overlay, or even redirecting directshow output to a completely different display device. In the case of things like ffdshow the codec itself does a lot of these things. Most likely if the user has their own codec for a given format installed then they will probably want to use it, even if there is no end-result difference.
Comment 7 cajbir (:cajbir) 2008-07-03 16:00:05 PDT
Created attachment 328056 [details] [diff] [review]
Version 2 of the Theora/Vorbis video/audio backend

Updated to apply on mozilla-central and work with recent changes to the patch on bug 382267
Comment 8 cajbir (:cajbir) 2008-07-23 10:32:43 PDT
Created attachment 330955 [details] [diff] [review]
Adds libogg-1.1.3 to modules subdirectory

This patch adds libogg-1.1.3 to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.
Comment 9 cajbir (:cajbir) 2008-07-23 10:35:15 PDT
Created attachment 330957 [details] [diff] [review]
Adds libvorbis-1.2.0 to modules subdirectory

This patch adds libvorbis-1.2.0 to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.
Comment 10 cajbir (:cajbir) 2008-07-23 10:37:16 PDT
Created attachment 330959 [details] [diff] [review]
Adds libtheora-1.0beta3 to modules subdirectory

This patch adds libtheora-1.0beta3 to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.
Comment 11 cajbir (:cajbir) 2008-07-23 10:39:12 PDT
Created attachment 330960 [details] [diff] [review]
Adds liboggz-0.9.8 to modules subdirectory

This patch adds liboggz-0.9.8 to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.
Comment 12 cajbir (:cajbir) 2008-07-23 10:41:15 PDT
Created attachment 330961 [details] [diff] [review]
Adds libfishsound-0.9.1 to modules subdirectory

This patch adds libfishsound-0.9.1 to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.
Comment 13 cajbir (:cajbir) 2008-07-23 10:44:32 PDT
Created attachment 330962 [details] [diff] [review]
Adds liboggplay to modules subdirectory

This patch adds liboggplay and the liboggplay audio files to the modules subdirectory. A security review of this code has been requested.

I only include the parts of the original libraries that I actually use. The encoders have been stripped out so only the decoding code remains (as much as I could). I include a README_MOZILLA file that explains the source and version of the libraries, and an update.sh script for each library that when run with the path to the original library source it copies the relevant files and applies any patches needed to build.

liboggplay doesn't have a release build yet. I've used r3602 from the SVN and r3605 for the audio code. This is explained in the README_MOZILLA files in the relevant directories.
Comment 14 cajbir (:cajbir) 2008-07-23 10:50:10 PDT
Created attachment 330963 [details] [diff] [review]
Version 3 of the Theora/Vorbis video/audio backend

This patch no longer requires the separate download mentioned in comment 1. Instead it needs the various third party module patches I've added to this bug.

It requires the patch in bug 447639 to be applied as it has been updated to use the latest WHATWG spec names for things.

This patch removes the --enable-media configure option and instead requires --enable-ogg. By using --enable-ogg it will automatically set MOZ_MEDIA (which is what --enable-media used to do). This addresses a note made in bug 382267 comment 97. 

Not all video/audio WHATWG features are implemented. The plan is to add these features under separate bugs once this is ok to land.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-23 13:08:40 PDT
bsmedberg (:bs) should review the build changes.

-    TK_LIBS='$(MOZ_GTK2_LIBS)'
+    TK_LIBS='$(MOZ_GTK2_LIBS) -lasound'

Seems like we need a configure check for libasound. If it's not available either we fail to build or we disable MOZ_MEDIA ... I guess it depends if libasound is ubiquitous or not.

+      mDecoder = new (std::nothrow) nsOggDecoder();
+              mDecoder = new (std::nothrow) nsOggDecoder();
(and elsewhere)

We don't use std::nothrow usually so we probably shouldn't use it here.

+      if (NS_SUCCEEDED(source->HasAttr(kNameSpaceID_None, nsGkAtoms::src))) {

HasAttr returns a PRBool, not an nsresult.

+        if(NS_SUCCEEDED(rv)) {

Space needed in "if("

+          if (type == NS_LITERAL_STRING("video/ogg") || type == NS_LITERAL_STRING("application/ogg")) {

Use EqualsLiteral. Or even better, use FindAttrValueIn.

+            if (NS_SUCCEEDED(source->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {

GetAttr returns PRBool too.

+  gAudioStreamLog = PR_NewLogModule("nsAudioStream");

Should be #ifdef PR_LOGGING I think.

nsChannelToPipeListener needs to use the a* convention for parameter names.

+    mTotalBytes += count;
+    do {
+      count -= bytes;
+      nsresult rv = mOutput->WriteFrom(stream, count, &bytes);
+      NS_ENSURE_SUCCESS(rv, rv);

So we update mTotalBytes with 'count' even if we fail to write that many bytes. That doesn't seem like the right thing to do. I think we should try to write 'count' bytes and report how many we wrote via UpdateBytesDownloaded.

+  nsresult rv = mInput->Read(buf, n, &bytes);
+  if (!NS_SUCCEEDED(rv)) {
+    bytes = 0;
+  }

Do we need to do a loop here to handle incomplete Reads?

+  nsChannelReader * me = (nsChannelReader *)opr;

static_cast

+  /******
+   * The following member variables can be accessed from any thread.
+   ******/
+  

For these variables you should document how thread-safety is achieved. Either they're read-only while multiple threads are running, or they're protected by some lock (say which).

+  // Total number of bytes downloaded so far
+  PRUint32 mBytesDownloaded;
+

Move this above the PRPackedBools to save 4 bytes...

+    if ((r != E_OGGPLAY_CONTINUE && 
+         r != E_OGGPLAY_USER_INTERRUPT && 
+         r != E_OGGPLAY_TIMEOUT)) {

Don't use double parens

+void nsOggDecoder::BufferData()
+      // sleeping the decode thread, not the main thread. We stop
+      // sleeping when the resource is completely loaded, or the user
+      // explicitly chooses to continue playing, or an arbitary time
+        PR_Sleep(PR_MillisecondsToInterval(1000));

So there's no early wakeup here, so if the user explicitly chooses to continue playing there's going to be a delay of up to 1s?

+    if (mReader->Available() <= 2048 && !mResourceLoaded) {

So this buffering isn't all that smart, right? If we receive a few K of data at a time (less than one frame, I presume) we won't ever start buffering.

+  SetRGBData(y_width, y_height, mFramerate, nsnull);
+  {
+    nsAutoLock lock(mRGBLock);

So is there a race condition here where we can render uninitialized data? that seems bad.
Comment 16 cajbir (:cajbir) 2008-07-29 17:50:59 PDT
Created attachment 331682 [details] [diff] [review]
Version 4 of the Theora/Vorbis video/audio backend

bs, I've added you for review of the configure changes at roc's request. Additions are to handle --enable-ogg and to autodetect libasound (Alsa sound library) on Linux. If the library doesn't exist, and ogg support is requested, the configure fails.

This patch addresses the review comments by roc.

Only change not made is:

> Do we need to do a loop here to handle incomplete Reads?

No, an incomplete read is valid and handled by liboggplay (which is the caller of this routine).

> So there's no early wakeup here, so if the user explicitly chooses to continue
> playing there's going to be a delay of up to 1s?
> ...
> So this buffering isn't all that smart, right? If we receive a few K of data at
> a time (less than one frame, I presume) we won't ever start buffering.

Buffering is now changed to check the download rate over a period of time and buffer if the read will starve during that time based on the current download rate.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-29 22:39:26 PDT
Comment on attachment 331682 [details] [diff] [review]
Version 4 of the Theora/Vorbis video/audio backend

I got r=ted in person during a Rock Band break
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-29 23:47:41 PDT
Pushed libraries to changeset b6e9bc7d95bb.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-29 23:53:00 PDT
Pushed integration patch to changeset f6b43d90489f.

We need tests here though.
Comment 20 Biju 2008-08-02 21:59:02 PDT
Why cant we also play links to *.ogg files like we do for *.mpg or Quicktime files

Bug 448907
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-05-28 21:38:37 PDT
We have a number of Ogg Theora reftests.
Comment 22 Henrik Skupin (:whimboo) 2009-05-29 03:09:40 PDT
Marking verfied fixed since it went in nearly a year ago.

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