[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #29120 [Applications/Tor Browser]: Default value of media.cache_size (0) causes some media to load extremely slowly or become unplayable

#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs