Last Comment Bug 855064 - Exclude DASH from the build by default
: Exclude DASH from the build by default
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Steve Workman [:sworkman] (INACTIVE)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 14:17 PDT by Chris Pearce (:cpearce)
Modified: 2013-04-03 03:44 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1.0 Disables DASH build by default; excludes more code via MOZ_DASH (15.26 KB, patch)
2013-04-01 15:02 PDT, Steve Workman [:sworkman] (INACTIVE)
cpearce: review+
Details | Diff | Splinter Review
v1.0 Removes DASH references from media mochitests (12.84 KB, patch)
2013-04-02 09:59 PDT, Steve Workman [:sworkman] (INACTIVE)
cpearce: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2013-03-26 14:17:06 PDT
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.
Comment 1 Ralph Giles (:rillian) needinfo me 2013-03-26 14:27:20 PDT
Can you document why we're not going to enable it?
Comment 2 Chris Pearce (:cpearce) 2013-03-26 14:57:23 PDT
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.
Comment 3 Steve Workman [:sworkman] (INACTIVE) 2013-03-26 14:59:42 PDT
(I was writing this comment, but Chris posted before I did - this mostly echoes what he wrote).

Basically, it's a combination of a resource/prioritization issue and momentum growing for Javascript DASH implementations on top of MediaSource. Since development started, the JS style implementations have emerged as more desirable than native DASH (C++ in Gecko) for the big players. We currently don't have the resources to warrant spending more time on having a second approach, so work has been halted.
Comment 4 Ralph Giles (:rillian) needinfo me 2013-03-27 15:45:26 PDT
Thanks for the explanation. Here's hoping those industry drivers don't change their minds again.
Comment 5 Steve Workman [:sworkman] (INACTIVE) 2013-04-01 15:02:31 PDT
Created attachment 732076 [details] [diff] [review]
v1.0 Disables DASH build by default; excludes more code via MOZ_DASH

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.
Comment 6 Chris Pearce (:cpearce) 2013-04-01 15:56:33 PDT
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.
Comment 7 Steve Workman [:sworkman] (INACTIVE) 2013-04-02 09:59:07 PDT
Created attachment 732394 [details] [diff] [review]
v1.0 Removes DASH references from media mochitests

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
Comment 8 Steve Workman [:sworkman] (INACTIVE) 2013-04-02 17:06:23 PDT
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 9 Steve Workman [:sworkman] (INACTIVE) 2013-04-02 17:06:43 PDT
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

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