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.

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.

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?

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?

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

marking wanted1.9.1? to get this in the triage queue. If this needs to be blocking1.9.1?, please mark it as so.

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

Updated to apply on mozilla-central and work with recent changes to the patch on bug 382267

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.

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.

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.

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.

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.

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.

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.

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.

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

Pushed libraries to changeset b6e9bc7d95bb.

Pushed integration patch to changeset f6b43d90489f. We need tests here though.

Why cant we also play links to *.ogg files like we do for *.mpg or Quicktime files Bug 448907

We have a number of Ogg Theora reftests.

Marking verfied fixed since it went in nearly a year ago.