#29120: Default value of media.cache_size (0) causes some media to load extremely slowly or become unplayable -------------------------------------------------+------------------------- Reporter: QZw2aBQoPyuEVXYVlBps | Owner: tbb- | team Type: defect | Status: | needs_revision Priority: High | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: tbb-disk-leak, tbb-usability- | Actual Points: website, TorBrowserTeam201902 | Parent ID: | Points: Reviewer: | Sponsor: -------------------------------------------------+-------------------------
Comment (by gk): Replying to [comment:20 QZw2aBQoPyuEVXYVlBps]: > Replying to [comment:19 gk]: > > You have > > {{{ > > if (aContentLength < 0) { > > }}} > > in your patch but looking at the original code I think we should take care of the case here as well that `aContentLength` is `0`. > If you look here: https://hg.mozilla.org/releases/mozilla- esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l153 > You'll see that `aContentLength` is `-1` if it is unknown, so an `aContentLength` of `0` means that's what the server actually sent. Yes. I am a bit wary that we are subtly changing the logic here: right now in Firefox esr60 a memory-backed media cache is only used if `aContentLength` is larger than `0` and less or equal than the max size as determined by a pref. Your patch keeps the latter requirement. However, it changes the former exposing the memory cache creation to a content length of `0`. While this seems okay for now I am a little bit worried that Mozilla changes something under the hood which is not affecting them as they have the `>` check while we don't exclude the case where `aContentLength` is `0`. So, I guess without making your patch even more complicated, one way forward would be to take it in the alpha and try to upstream it at the same time, so we are sure we won't get surprised by our logic change. That's fine with me. > > Additionally, I think we should go back to the default size for `media.cache_size` so that users outside of private browsing mode get the usual behavior (right now they would still be affected by this bug without resetting the pref). (https://gitweb.torproject.org/tor- browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor- browser-60.5.0esr-8.5-1 is the file to look at) > I agree with this, I was going to mention it but it slipped my mind. I also suggested above that the default of `media.memory_cache_max_size` should be increased to handle buffering longer media content in memory, what do you think about this? We could do that. But I'd be cautious here. Maybe let's pick twice as the default for now? That should not be worse than the status quo and we essentially have not heard about the bug you reported before. So, the problem might not be that widespread. > > QZw2aBQoPyuEVXYVlBps: Thanks for this find and a patch, really appreciated. We are close here. Do you want to fix the remaining things up or should we? How should we credit you? (I am fine crediting QZw2aBQoPyuEVXYVlBps in the commit message :) ) > It's a little time consuming for me to test my code since compiling takes awhile, but since there's only a few changes, I think I should be fine completing it. > You can credit either "QZw2aBQoPyuEVXYVlBps" or "anonymous", this is simply a throwaway account because I wasn't able to get the cypherpunks account to work. Okay, let's use "cyperpunk" then. :) Thanks for you work here and testing, much appreciated. If you could change the pref I am happy to merge the patch and test it wider in an upcoming alpha. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/29120#comment:22> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs