Closed Bug 855064 Opened 7 years ago Closed 7 years ago
Exclude DASH from the build by default
Steve has done a lot of great work on adding DASH support to Firefox, and it's a damn shame that we're not going to be enabling it. We've talked about it amongst the media team here in Auckland, and since we're not going to be turning DASH on, we'd like to disable DASH in the build and let it bitrot. This is to reduce our maintenance burden; when we do any refactorings we don't want to also have to do them to the DASH backend as well. The DASH backend required quite invasive changes to the MediaDecoder architecture to get working, making MediaDecoder harder for us to refactor and maintain, which we want to do going forward. Disabling DASH will also make our tests run faster, and thus reduce the turn around time of tinderbox and local mochitest runs. I'm not proposing that we remove the DASH code at this stage, it serves as a good guide for the work on RTSP. However we would like to remove the DASH code after a while if it isn't found to be needed any more.
Can you document why we're not going to enable it?
I'm not the one who made the decision so I don't have detailed knowledge, as I understand it the industry drivers that were pushing for DASH decided that they'd rather have MediaSource, and there are JS libs out there that can be used to get DASH support anyway.
Thanks for the explanation. Here's hoping those industry drivers don't change their minds again.
I think I got most of the DASH code here. I added some #ifdefs in ChannelMediaResource, one for PrepareToDecode, and WebMReader. Let me know if there's something else you can think of.
Attachment #732076 - Flags: review?(cpearce)
Comment on attachment 732076 [details] [diff] [review] v1.0 Disables DASH build by default; excludes more code via MOZ_DASH Review of attachment 732076 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Steve. ::: content/media/MediaResource.cpp @@ +66,5 @@ > mIsTransportSeekable(true), > mSeekOffsetMonitor("media.dashseekmonitor"), > mSeekOffset(-1) > +#else > + mIsTransportSeekable(true) Just move mIsTransportSeekable outside of the MOZ_DASH block, no need to declare it inside and out with the same initializer.
Attachment #732076 - Flags: review?(cpearce) → review+
Mochitest-1 failed on try yesterday due to references to DASH files that are no longer there. I removed them in this patch and pushed to try again, with M1 passing. https://tbpl.mozilla.org/?tree=Try&rev=4342ca733525
Assignee: nobody → sworkman
Status: NEW → ASSIGNED
Attachment #732394 - Flags: review?(cpearce)
Attachment #732394 - Flags: review?(cpearce) → review+
Comment on attachment 732394 [details] [diff] [review] v1.0 Removes DASH references from media mochitests https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9087d86d45
Comment on attachment 732076 [details] [diff] [review] v1.0 Disables DASH build by default; excludes more code via MOZ_DASH https://hg.mozilla.org/integration/mozilla-inbound/rev/81d4bd15053d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.