mozilla :: #media

9 Oct 2017
11:57padenotpehrsons, hrm your test does not fail
12:15pehrsonspadenot: I know :/
12:15padenotpehrsons, do you know why ?
12:16pehrsonswith module-sine-source locally the distortions are much less frequent, the analyser barely picks them up
12:16pehrsonsseems like callbacks are less frequent too, but not that much
12:16pehrsonsthere are still distortions fwiw
12:17padenotwith your patch?
12:19pehrsonspadenot: without the fix
12:20padenotok
12:20pehrsonsjust because we're using the sine source instead
12:20padenotyeah
12:20pehrsonsso while my microphone had an input callback every ~130 frames, the sine source gives me one every ~7-8k frames
12:21padenotinteresting
12:21pehrsonsso out of 8k samples, around 50 are null
12:21padenotharder to test for
12:21padenotwhat is the pulse module to load ?
12:22pehrsons`pactl load-module module-sine-source`
12:26padenotyeah so it defaults to 2s of latency
12:27padenotif we use the loopback/monitor device instead, we can configure the latency
12:29padenotor maybe we can config it manually
12:29pehrsonspadenot: eh, with the default null monitor I get between 1 and 5 frames locally
12:29pehrsonsseems small
12:29pehrsonsbut would work for this test, hehe
12:29padenotyeah
12:30padenotI mean, in the longer term we'll want to move away from sine waves
12:30padenotif it's not too hard maybe it's worth it
12:30padenotdoing it now I mean
12:31pehrsonsthe only concern I've come up with with the monitor approach is that it will pass through the AEC on its way to the null sink
12:31pehrsonsnot sure how problematic that could be
12:32pehrsonspadenot: so can I put the test in a followup and hang it off of achronop's bug to replace the sine-source with a monitor?
12:33padenotpehrsons, not if you disable AEC using the constraints !
12:35pehrsonspadenot: yeah but it also means for AEC testing that you cannot add a component to the signal that is not known by the AEC as output
12:35pehrsonsnarrow enough? :-)
12:36padenotyou can with an HTMLMediaElement I suppose
12:36padenotit's not mixed in the reverse stream
12:36pehrsons"reverse stream"?
12:37padenotthe AEC has a reverse-stream (the output from the machine) and an normal stream (the mic, to process)
12:37padenotif you use an HTMLMediaElement, it's going to be picked up by the monitor, but unknown to the MSG, since it's not a MediaStream
12:37pehrsonspadenot: ah, you mean playback version of a media element :-)
12:37padenotthe monitor = the pulse monitor device
12:37padenotyes a normal one
12:37padenotor we can have a second MSG
12:38pehrsonsthat's a hack that would work for now
12:38pehrsonspadenot: in any case, are you ok with me putting the test in a followup?
12:38padenotin the long run we'll be able to control the number of MSG from content
12:38padenotpehrsons, well, as long as it's deal with "soon" yeah
12:39padenotI hear that achronop is going to do some testing stuff
12:39pehrsonssure
12:39pehrsonsI'll chase after achronop until we're monitor based
12:45jesupdrno|irccloud: Have you looked at bug 1402510 (Alexander Abagian's bug)?
12:45firebothttps://bugzil.la/1402510 NEW, nobody@mozilla.org [WebRTC] Incoming video is not shown for some of conference participants
12:54padenotjesup, fyi, https://reviewboard.mozilla.org/r/187628/diff/1#index_header
12:55jesuplooking
13:01padenotpehrsons, have you thought about directly using the mixer ?
13:01pehrsonspadenot: yeah I'll patch that up now
13:02padenotcool
13:03jesuppadenot: see https://stackoverflow.com/questions/2576022/efficient-thread-safe-singleton-in-c and the second comment there for a way to thread-safely initialize a singleton GetFoo() (in C++11 and supported (finally) in MSVC 2015)
13:03* jesup wonders how many times we used that before switching to MSVC 2015...
13:04jesupaka "magic statics"
13:04jesupthat may not help here, since you want to get rid of it when not needed
13:05padenotjesup, yes, that's know, we leverage this in multiple other locations
13:05padenotknown*
13:06jesupThough a static SharedThreadPool will shut down the thread after 60 (or configurable) seconds, just costing the size of the SharedThreadPool itself
13:07padenotmy point was mostly about the possible shutdown hang
13:13jesuppadenot: there's probably a way to use the static.. but not worth it. r+'d
13:14padenotoh it was just fyi, I'm going to review it
13:14padenotstatics are very bad
13:14padenotthey are part of the reason we're in such bad shape
13:18jesupI meant using the "magic statics" to implement thread-safe singletons in our system. Thread-safe singletons are Good Thing, and it's nice when they're efficient.
13:19padenotwe should actively work to minimize the number of singletons
13:19padenotit's low-traffic enough that performance is not a concern
13:19padenotquite nice to write though
13:20padenotfor now, we'll keep gGraphs
13:20padenotit should hang off the document at some point
13:39noxhttps://github.com/w3c/media-source/issues/200 :(
13:42jesuppadenot: please review independently (I didn't notice the r?padenot on that; thought your were pinging me to review it
13:42padenotjesup, yes I had planned to r+ before, just thought you should be aware, since we spent ages trying to find those
14:14cpearceJamesCheng: content_decryption_module.h has changed upstream. We'll need to resync sometime soon: https://cs.chromium.org/chromium/src/media/cdm/api/content_decryption_module.h?l=76&rcl=6e4c388c0117fe408b66fbede91081fb1018c5fe
14:50JamesChengcpearce: yes, I've noticed that and I think we should update it when we start to update to cdm v9
14:51jyabwu: how do you read that page ?
14:51jyahttps://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-10-05&keys=atiumd64.dll%2520(9.14.10.1197)!atiumd64.dll%2520(7.14.10.911)!igd11dxva64.dll%2520(20.19.15.4352)!igd11dxva64.dll%2520(20.19.15.4300)&max_channel_version=beta%252F57&measure=VIDEO_UNBLACKINGLISTING_DXVA_DRIVER_RUNTIME_STATUS&min_channel_version=null&processType=*&pro
14:51jyaduct=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-09-25&table=1&trim=1&use_submission_date=0
14:51jyahttps://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-10-05&keys=atiumd64.dll%2520(9.14.10.1197)!atiumd64.dll%2520(7.14.10.911)!igd11dxva64.dll%2520(20.19.15.4352)!igd11dxva64.dll%2520(20.19.15.4300)&max_channel_version=beta%252F57&measure=VIDEO_UNBLACKINGLISTING_DXVA_DRIVER_RUNTIME_STATUS&min_channel_version=null&processType=*&pro
14:51jyaduct=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-09-25&table=1&trim=1&use_submission_date=0
14:52jyahttp://tinyurl.com/ycmghbcl
14:52jyahow do you tell if the device has crashed or not?
14:53bwujya: change "advanced setting" by setting "Show tables" & "Non-cumulative"
14:54jyait's already set that way
14:54jyai see lots of start 0, 1, 2, 3, 4 .... and end 1,2,3,4, ...
14:54jyaThat doesn't tell me if it has crashed or not
14:55bwuheh. That's the tricky part.
14:55jyause the tinyurl link I posted above, they other were truncated
14:55bwustart 1 end 2 --> crash numbers
14:56jyawhat's the difference with start 0 end 1 ?
14:57bwustart 0 end 1 -> remote HW-decoding and GPU process didn't crash
14:58bwustart 1 end 2 -> remote HW-decoding and GPU process crashed.
14:58jyahard to tell from the top description which is "0:remote HW-decoding and GPU process didn't crash, 1:remote HW-decoding and GPU process crashed"
14:58jyaso 0 didn't crash, 1 crashed
14:58jyanot 1 and 2
14:58bwuI know. :-)
14:58bwuit is hard to tell
14:59jyagood to see that the crash account for a rounded number of 0%
14:59bwuha
14:59jyabut having said that, isn't that big numbers not the numbers of instance, but the number of times a video got started on windows
15:00jyaI seem to recall that it's recorded whenever a new decoder is created
15:03bwuyes. If I understand correctly about what you mean, you are right. That number doesn't mean the number of users but means how many times video decoder is created to be used without crashes.
15:04padenotjib, ping
15:04jibpadenot: pong
15:05padenotjib, MediaManager is global per process, is that an important thing to conserve ?
15:07jibpadenot: For non-e10s maybe
15:07jibShort of some reason I may have overlooked, I don't see why per-process globals should ever be needed in our new architecture
15:07padenotright
15:08jibpehrsons rewrite a lot of our UX hooks to be per window already, but then MediaManager.cpp is big, so it's hard to say for sure
15:08jibrewrote
15:10pehrsonsLet's try to kill the singleton and we shall see what happens?
15:10jibThe real synchronization point is the media manager thread
15:11jibin same-process
15:12padenotsynchronization point between what ?
15:15* jib is trying to picture multiple MediaManagers all sending work to the media thread. Should work, except we might have a problem with outstanding requests, where we today use MediaManager::GetIfExists() (not consistently, though I have a patch for that) to drop obsolete requests after OnNavigation or shutdown
15:16padenotwhy do we need a single media thread ?
15:17jibyou're thinking a media thread per tab that has called gUM or enumerateDevices ever?
15:17jibwhy do we have a single main thread? :)
15:17padenotwe don't!
15:17padenotwe have multiple process now
15:18jibI mean in within the same process
15:18padenotit's a artifact of history
15:18jibyes
15:18padenotwe're talking about making things better, so I'm asking questions :-)
15:19jibI'm using a devil's argument I suppose by applying the same rationale to the main thread, which is still a singleton within any content process
15:19padenotnot for long, no
15:19jibin order to help me think about whether one media thread or several within the same process makes sense or not
15:19padenotthere are still going to be linearization points, though
15:20padenotalso, why do we have a permanent MediaManager thread, is that useful ?
15:20padenotfor audio it's not too useful, but audio is only half the story
15:21jibFor video everything gets done through CamerasParent I think (or close to it)
15:21jibHistorically, having a single thread avoids locking a lot of memory
15:21padenotis that a different thread? or is it using the MediaManager thread ?
15:22jibStill the MediaManager thread in the content process
15:22jibIsn't the choice of single thread vs. thread pool orthogonal to the question of a singleton vs. per-window (document?) in the same process?
15:24padenotyes, although it's easier to have mutable global shared state if you share some stuff
15:25jiband also the quantum runnable labeling doesn't require separate threads, right?
15:25jibin order to be performant?
15:25jyajw_wang: you really should need to look into reports a bit longer before dismissing them outright.
15:25jyaRf bug 1397708
15:25firebothttps://bugzil.la/1397708 UNCONFIRMED, jwwang@mozilla.com Video tag element is not firing progress event after some time at first play
15:26padenotjib, it does not require it, but it allows it
15:26jibright
15:26jyaIf you just ran the JS provided, you would see that it could go on for over 30s of download, and not getting any progress event
15:26jibit would be an interesting experiment
15:26jw_wangjya, I can't. my network is so slow now it doesn't download anything.
15:27jyajw_wang: then don't close the Ni? Dismissing the information
15:28jyaI wouldn't Ni? You if I didn't find a genuine problem. It's highly frustrating
15:29jw_wangno, I can't see the problem
15:29jibpadenot: I *think* we'd get all the same benefits with a single media thread, which seems superior for memory footprint. Seems uncommon to want to blast gUM from multiple tabs at once (except for devs)
15:30jw_wangas I explained, you can't relate buffer ranges to the 'progress' event
15:30jibon the other hand, if it's uncommon, then so is the extra memory footprint perhaps
15:30padenotjib, this is really not hot, I'm thinking more about maintainability here
15:30padenotjib, yeah
15:30jibisn't maintainability the same?
15:30padenotjib, nothing concrete, I'm toying with designs, but of course I mostly know about issues related with audio
15:31jibwrt threads I mean
15:31padenotjib, you need to maintain locking if you have the same thread
15:31padenotyou don't if you have a thread per document
15:31jibwhy? if we use runnables then the runnables are the sole locking I think
15:32jibsorry sole
15:32jyajw_wang: the spec says that progress should be fired every 350ms or every bytes downloaded, whichever comes last
15:32solejib: pardoned haha
15:33padenotjib, true, but only if we have fixed our mutable shared state protected by mutexes, first
15:33jibyes
15:34jyaWe fire progress when networks states move to NETWORK_LOADING
15:34jyaWe never fire the progress event after that, even though we're clearly downloading. It doesn't matter how long you wait
15:35jyaLoading the page fires a progress event once. That's just to display the first frame, and the first 10s are loaded.
15:35jyaProgress is fired about twice during that time, even though it takes over 3s here to download the first 11s
15:36jyaWhen you press play, and playback resume, progress is fired right after you press play, but never again.
15:36jyaAfter a minute or so, we have over 90s of data buffered, no progress event were fired during this time
15:37jw_wangmaybe we just download the whole file in one shot, so only one event is fired.
15:37jyaLuckily the reporter is persistent, so when his bug is closed as invalid with no explanation, he doesn't give up and reopen the bug
15:37jyaThat is no what the specs states
15:37jyaEven if you downloaded in one shot
15:38jya"While the load is not suspended (see below), every 350ms (200ms) or for every byte received, whichever is least frequent, queue a task to fire an event named progress at the element."
15:39jyaI had people at demuxed telling me they had to make special code for Firefox, because we didn't fire the progress event.
15:40jw_wangif your download is finished in one shot, the load is suspended immediately, right?
15:40jyaSo unless you download the 90s video in less than 350ms, progress should be fired
15:41jyaBut it's not downloaded in one shot, you can see over the course of one minute in the user log that buffered change over 1 minute, by blob of 20-30s, yet no progress event is fired
15:41pehrsonsjib: padenot: there's some blocking done by CamerasChild, so perhaps we don't want to limit ourselves to one thread per process. Or we make CamerasChild async with something promisey and we'd probably be fine. We could also look at a small thread pool and a TaskQueue per document. That seems like the least we should do.
15:42jibso task queue would make sense if we worry about performance of concurrent access. Not sure that's pressing
15:43jibpadenot: so one maintenance benefit I could see from having a media thread per MediaManager, is that it would mean we could continue the invariant of (the relevant) MediaManager existing on the media thread. We might also be able to fold some cleanup code in shutdown and OnNavigation, we have lots of ugly checks since obsolete runnables may be on the mainthread queue
15:45jw_wangjya, buffered ranges are updated asynchronously, so when you see changes in buffered ranges, it doesn't necessarily mean there are data being downloaded.
15:48jw_wangchances are we've downloaded all bytes, but buffer ranges are not updated yet.
15:52jyajw_wang: when I see buffered range changing over the course of ONE minute, the asynchronous reason, or the 500ms potential delay sounds like rather poor interpretation
15:53jyaNever mind, I'll look into it myself. I'm not supposed to do media anymore.
15:54jyaThe CHANCES are ZERO that we've downloaded it all in less than 350ms.
15:54jyaBecause if it then takes one minute to update the buffered range, we have something else broken
15:55* jya highly frustrated now.
15:55jw_wangwhere is the ONE minute from?
15:59jyajw_wang from the user log, and from my logs.
15:59jyaHis script sets a timer that shows the value of the buffered attribute every 4s
16:00jyaYou see the value of the buffered attribute changing over the course of ONE minute, it keeps increasing, yet buffered is only fired once when you press play()
16:00jw_wangThere are only 5 samples of the logs which is 5x4=20s
16:00jyaOn my logs it took 1 minute
16:00jyaAnd even if it was 20s
16:01jya20s > 350ms
16:01jibpehrsons: What if we had a way to drop runnables of a certain class (name) on the mainthread queue? Does MozPromise have anything like this?
16:04pehrsonsjib: with MozPromise you tell it explicitly where to run
16:04jyajw_wang: it's more obvious if you change the timer from 4000ms in the script, to 400
16:05jibpehrsons: yeah but is there a way to cancel the whole thing?
16:05jibwithout lots of state checks in every callback
16:05pehrsonsoh that kind of dropping
16:05* jib is looking for invariants
16:05jyajib: you never cancel a task with MozPromise, nor with any task queues. You can only disconnect the promise indicating that you're no longer interested in the result
16:06jyaWe used to have a cancellable task queue, but this was removed from the code (as it should be)
16:06jibjya: ok but this disconnect prevents further callbacks (runnables) from being called?
16:12jibA check in every callback works fine, it's just a maintenance problem we have in MediaManager with lifetimes, when navigating away or shutting down.
16:12jibone more state
16:15Caspy7someone here is asking how to know if HWA is enabled in 57+ https://www.reddit.com/r/firefox/comments/757lg6/how_to_know_if_hardware_acceleration_is_turned_on/
16:16Caspy7what would be the answer to that?
16:22pehrsonsjib: haven't used it but looks ok: http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/xpcom/threads/MozPromise.h#120
16:23jibcool
16:27* jib wishes this weren't so: "A MozPromise is ThreadSafe, and may be ->Then()ed on any thread."
16:28pehrsonstalked to jw_wang yet? :-)
16:28jibnot yet :) sorry
16:29jibI should probably take some time to play with it first
16:32pehrsonsI used it here if you want a starting point: http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/MediaRecorder.cpp#1044
16:32jibthanks!
16:34jyajib: depends on what you mean by being called.
16:34jibrun
16:35jyaThe original task you ran (that returned a promise) will resolve or reject the promise as if it hadn't been disconnected.
16:35jyaBut that will now stop, as the promise is dksconnected
16:36jyaUsing MozPromise in place of callbacks is significantly easier to handle. It makes the code easier to read, less likely to have threading issues (as callbacks typically are called from another thread)
16:37jibright
16:37jyaIn fact, it's so much better, this is a big chunk of what I'm planning to do. Replace synchronous tasks with asynchronous one where possible. Remove callbacks and use promises instead
16:37jibjya: just be sure to file a bug first ;)
16:39jibjya: note I'm aware of the benefits of promises, I even wrote a similar class (media::Pledge) before MozPromise landed. I've had some concerns switching over, but they're mostly nits on MozPromise
16:39jyaOf course, like always, I do the changes, then file the 10+ related bugs :)
16:39jibjya: that's not ideal
16:39jyaYeah, we had this argument last year of pledge VS MozPromise
16:40jyaThat the rsolution queue new task was the downer for you.
16:43jibjya: It's more that the object itself is always thread-safe, which seemed odd to me. I'm not passing a promise object to a different threads so they can add .Then() to it
16:44jibIt may boil down to that I'd love to have a .Then(callback) overload without the thread argument
16:46jibit may be fine, I suspect I need to play with it
16:57jya you always have access to the information about the current thread running
16:57jibpehrsons: review up on bug 1406988
16:57firebothttps://bugzil.la/1406988 NEW, nobody@mozilla.org Assert MediaManager invariant where applicable, and check for existence everywhere else.
16:58jyaproviding the thread object allows to handle the lifetime of the thread/taskqueue
16:58jibjya: I meant as a shortcut to mean "same thread".
16:58* jib brb
17:01jyaThen() ensures that the taskqueue is kept alive... I guess Then() could call AbstractThread::CurrentThread() by default... lodge a bug :)
17:02jyathough, to me it makes the code less explicit.
17:02jyathat also forces you to think about threading model
17:10jibjya: It may be a JS hangup of mine. I was hoping to encourage a code-pattern of same-thread Then()-chains, much like in JS, with all different-thread operations in the sub-functions returning the promises. I see from pehrsons patch that we use MozPromise also as a task scheduler effectively, so it's quite different from JS. If this al boils down to ergonomics, then my point doesn't amount to much perhaps, but I haven't shaken the feeling that there
17:10jib may be some redundant locking. As I said I should find time to check it out and stop complaining until I've verified this
17:11jyajib: there is indeed locking in place as the task is queued. TaskQueue uses a monitor for that (which allows for waking up the running thread/taskqueue)
17:12jibyes there needs to be some locking of course since the whole point is to do something asynchronous, just need to a acquaint myself and compare how much
17:14jibI guess I wonder why the MozPromise objects are also thread-safe
17:16jibFor instance in pehrsons simple use of it, it doesn't seem needed: http://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#1044
17:19jibjya: if under the hood we're queueing runnables, why do we need to also lock e.g. when adding a .Then() handler? - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/xpcom/threads/MozPromise.h#114
17:30jyaSo that the MozPromise can be thread safe and can be resolved on another thread.
17:32jibAre we going to have multiple threads competing to resolve a MozPromise?
17:35mjfpadenot: ping
17:47jyajib: we may not in general, the fact is however is that a MozPromise is resolved on one thread, and it's resolution run in another. That itself means you need synchronisation.
17:47jibI thought we queued tasks instead
17:48jyaThere's one thing peculiar with MozPromise related code; while MozPromise is thread-safe, MozPromiseRequestHolder and MozPromiseHolder aren't.
17:48jibok good
17:49jyaI'm not sure where you confusion on the need to have a locking/sync mechanism comes from. Seems pretty obvious to me. Maybe I'm just too familiar with them.
17:50jibjya: Compare to http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/systemservices/MediaUtils.h#31,34
17:50jyaMozPromise can be shared multiple times, useful for doing things like All(nsTArray<MozPromise> then
17:51jyaThe code of MozPromise is there, that&#39;s the best explanation I guess.
17:52jyaYou also need to test if a promise has already been resolved or not, that test itself can be done on multiple queue.
17:52jyaBottom line, MozPromise are thread safe, pledge aren&#39;t.
17:53jibyeah but ideally we wouldn&#39;t have both
17:54jyaIn the most common usage of MozPromise, before the code of Then even gets to run, the promise has already been resolved, Then() will read the value of that promise, determine its resolved (read on another thread) and queue the task
17:54jyaThe whole purpose of MozPromise is to run tasks asynchronously, and do so on another thread if possible.
17:55jibI understand the purpose
17:55jyaSo I don&#39;t understand the point you&#39;re trying to make I guess.
17:58jibI&#39;m just curious about the design choice to use both locks and task queues. Seems like belts and suspenders to me.
17:58jibI&#39;ll work on my feedback
18:00jibIt would help if MozPromise, and all the needs it is trying to cover, were documented somewhere. Otherwise it&#39;s hard to offer feedback. &quot;bottom line&quot; as you say.
18:00jyaI don&#39;t see how it could be done any other ways... They aren&#39;t used for the same purpose.
18:01jyaI find the documentation rather well done.
18:02jyaAnd we&#39;re at a point of MozPromise development that we can ignore in how it does thing, and concentrate on what you can do with them
18:02jibjya: in the code or someplace else?
18:02jyaThe code. It explains what it does.
18:05jyaThe MozPromise itself doesnt need the code.
18:06jibI&#39;m not following
18:06jyaBut you can use the MozPromise with multiple targets, multiple Then
18:06jyaSorry, I meant to say The MozPromise itself doesn&#39;t need the mutex
18:07jibso why have it?
18:09jyaBut the Then does. Whenever you call Then, you in effect need to check if the promise has been resolved or not (can be done on multiple thread), and then append your tasks to the list. You don&#39;t want to queue the task just then, as the promise may not have been resolved yet.
18:09jyaSo that need your mutex
18:09drnoDoes anyone know why AEC logs are borken again?
18:10drnos/borken/broken/
18:10jyaSame when you resolve the promise, you need to check the list of pending then, and run them
18:10drnojesup: ^ ?
18:11jesupdrno: which system? Mac?
18:11drnojesup: yes in my/this case Mac
18:11jyaSo you have threads adding themselves to the list of clients, of the MozPromise on any possible threads, and MozPromise having to check on its own thread what those clients are. How can you not do this with locking?
18:11jesupwe need to bleeping fix the logs... Set the pref before starting, right?
18:11drnopref?
18:12drnoI start a call on webrtc-landing. open about:webrtc. click the start button. and some time later the stop button
18:12drnoit prints that something should be in /tmp/ but there is nothing
18:12drnoand I switched to non-e10s
18:13jesupAh... sorry. Thinking regular logs. (Though you can set a pref for aec logs before starting IIRC). In any case, in-about-webrtc AEC logs will never work with sandboxing on in e10s until we add something to allow (sandboxed) file-writing
18:13jesupEven the &quot;via a pref&quot; thing likely doesn&#39;t work in e10s/sandboxing for the same reason
18:14jesupneed to open N files with new names and write to them. Sandbox won&#39;t allow that of course
18:14drnojesup: I think we should evaluate padenots idea of logging this stuff to memory instead
18:14jyajib: MozPromise::mThenValues is what needs concurrent access.
18:15jesupYou can ask in #boxing - we&#39;ve talked to them about making a directory where we can write to from a sandbox. Alternatively add an IPC protocol for AEC logging. Logging in memory doesn&#39;t help unless you have a way to get it out
18:16jesupWhich could be a &quot;Save log to file&quot; button, but that adds to the fun since we need at least 3 separate logs (and really want all 6 or whatever). Which means tar/zipping it to save, added complexity
18:16jibjya: ok and mThenValues potentially contains callbacks from all kinds of threads, is that right?
18:16drnowell what ever we hold in memory should be relatively easy to offer in a download button, or?
18:16jesupyeah, that&#39;s waht I&#39;m saying - but there are N files per AEC log
18:16jyajib: you can use MozPromise like so:
18:17drnosure. make it a zip or tarball
18:17drnocurrently people have to tar it up anyways, because nobody wants to send 6 single files
18:17jesupThat&#39;s the simplest and least risky - log to memory, then have a save button. Mock the writing interface used by webrtc.org or some such to buffer it
18:18jyaRefPtr<MozPromise<T1,T2> p = FunctionReturningPromise();
18:18jibSay I call, p.Then() and it&#39;s racing with p.Resolve(). If Then() wins, there&#39;ll be a task queued eventually right, on the same thread, that will execute the function I&#39;ve adding? Is the concern that if my Then() call looses, then there&#39;ll be nothing left to queue a task to run it? Seems like we could potentially queue a safety task that checks for that condition. As to API expectations, I assume like the JS Promise API callbacks are never called
18:18jib synchronously, right?
18:19jyaP can be passed around, on different thread. Typically used when you&#39;re chaining promises and the 2nd chain is run from another thread
18:19jyajib: no, there&#39;s always a task queued.
18:20jyaIf the promise is resolved prior the Then running, a task to the Then callback is queued.
18:21jyaYou never disconnect a Promise
18:21jibright, hence me not seeing a need for the lock. On the example,
18:21jyaA promise is always resolved or rejected,
18:22jibPassing a promise around so other functions can call p.Then() is a bit of an anti-pattern imho. One rarely sees that in JS for instance.
18:22pehrsonsjesup: drno: can we use the same file backed blobs that MediaRecorder uses to store the aec logs?
18:22jyaIf you want the ability to disconnect, you use MozPromiseRequestHolder
18:22jibjya: seems more common to return a new promise, like pehrsons does in his patch for example
18:22jyaThis is not JS, and JS promise has no concept of threads,
18:22jesupthey&#39;re blobs, but they aren&#39;t zip files
18:23jyaJS isn&#39;t multi threaded (if you ignore worker)
18:23drnojesup: we zip what ever we have in memory when the user clicks the download button, or?
18:23jyaI haven&#39;t seen pehrsons patch, which bug is that?
18:23jesupAnd in practice we&#39;d expose the AEC log as a blob and then trigger a save operation I presume, which will prompt the user
18:23jibstill, inside a .Then() handler you&#39;re implicitly queued behind all previous actions of a then chain, so there&#39;s no need to explicitly chain off the same promise. Can&#39;t the code just create a new promise at that point and return that?
18:24jibseems more decoupled
18:24jyaNo you do not.
18:25drnopehrsons: can you point me to an example for the file backend blobs? I might have a look some time later
18:25pehrsonsjesup: give blobs to js which can zip them? :-) or is there a way to stream multiple files into a tar file?
18:26jibjya: here for instance pehrsons returns a new promise from a .Then() handler. Are you saying that&#39;s incorrect? http://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#1067
18:27jesupThere&#39;s zip support in-tree, not sure of details but likely can handle it, then put result in a Blob
18:28jyajib: that&#39;s promise chaining, the templates code allows to check what is being returned. This allows for knowing what the most outside return needs to return.
18:29pehrsonsdrno: http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/MediaRecorder.cpp#623
18:29pehrsonsbaku just refactored this so should be good and recent, we had our own layer in place of MutableBlobStorage before
18:33jyajib: when you return that second promise, and it is resolved, it will resolve the original promise.
18:34jyaThat alone is an example of a resolution trickling up between two different threads
18:35jibjya: ok sorry I misunderstood you example then. Are you saying the mutex in MozPromise exists merely to allow the promise objects themselves to be passed around as arguments on other threads? If so, then A) I agree they need mutexes obviously, as would any object passed this way, and B) That seems like an edge-case not worth locking down all MozPromises over
18:38jyaBut just there in the MediaRecorder you are accessing the MozPromise on multiple threads. When you chain, the Request is converted into a ChainValue
18:38jyaStep into the code, our. What happens when that second promise is resolved, you&#39;ll see
18:38jyaGotta do some readings with my son
18:39jibjya: ok thanks for your patience, I&#39;ll have a look later
18:51pehrsonsjib: Am I right in that you&#39;re interested in a MozPromise that is only accessed from one thread and thus instead of being locked down with a mutex would assert that all access happens on the owning thread? Or do you have other concerns?
18:51drnojesup, pehrsons: FYI turns out that I had media.webrtc.debug.aec_log_dir set to /tmp/ for convience. That no longer works.
18:52jesupah. They may have made the &quot;default&quot; directory writable in sandboxes already. ETOOMANYBUGS to remember
18:53jesupStill, having a download tarball/zipfile may be a better way to do this *anyways*
18:53drnototally agree
18:53drnoa short term thing might also be to igore the value in media.webrtc.debug.aec_log_dir as it most likely is not going to work these days with sandbox
19:00jibpehrsons: Initially I was, though that seems a bit limited. To be clear, I don&#39;t think any of the media code that uses Pledge today is much of a hot path to worry about. What I don&#39;t understand is why there needs to be any locking around MozPromise at all. I&#39;m just interested in having a good pattern.
19:04jibI should just look at the code
19:11jyajib: when you return a MozPromise like in http://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#1067
19:12jyathat is not a new promise per say.
19:12jyawhen that promise is returned, it is chained to the original MozPromise
19:12jyathat original MozPromise will be resolved in the thread where that 2nd MozPromise is returned
19:13jyait&#39;s the same as Doing MozPromise()->Then(...)->Then(...)
19:15jyayou fall into this case: http://searchfox.org/mozilla-central/source/xpcom/threads/MozPromise.h#125
19:15jyahow the promise is returned, internally makes it convert to another object, either a Request or a CompletionPromise
19:17jibI don&#39;t doubt the implementation is using locks
19:19jyaright now, when you chain the promise, the MozPromise::mMutex allows you to resolve that chained promise immediately.
19:20jyayou could I guess do that without lock, queuing an intermediary task to resolve the promise.
19:20jibWhat does that mean &quot;immediately&quot;? Is the .Then()) handler run immediately?
19:20jyabut you only made the handling more complicated
19:20jibright
19:20jibcomplicated for whom?
19:20jyaand you move the requirement for the mutex to the taskqueue&#39;s monitor anyway
19:21jibright, one thread-blocking lock is better than two
19:21jyaqueuing a task, running it, takes more time than taking the mutex and resolving the promise
19:21jyacould also argue it has a higher latency
19:22jibyou want to change from queueing a task to manage tasks solely through mutexes? :)
19:22jyai&#39;m not managing a task, I want to resolve a promise
19:23jyaall of this are implementation details.
19:24jibI&#39;ll look at the code
19:25jyathe MozPromise::mMutex also protects other members, and used for other mechanisms such as non-exclusive promises
19:25jyanot just chaining.
19:25jibWhere are those documented?
19:25jyathough, other than PromiseShutdown, I&#39;m not sure we have other non-exclusive Promise
19:26jyayou don&#39;t read c++ ???? :D
19:26jibI thought code was just implementation details? ;)
19:27jibhttp://searchfox.org/mozilla-central/source/xpcom/threads/MozPromise.h#140
19:27jyaI&#39;m going to have an early night... took me close to 30 hours to get back from SFO
19:28jibgood night!
21:32jesupFYI (derf jmspeex_ rillian): https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/6513488-ogg-vorbis-and-opus-audio-formats-support-firefox?tracking_code=38e82acf91277291102bdfbc15e96145
21:32jesupmoved to &quot;Working on it&quot; (broader OGG support)
21:33derfjesup: Yes, I saw that.
21:34derfhttps://developer.microsoft.com/en-us/microsoft-edge/platform/status/oggcontainer/ if you&#39;d like a link without a tracking code in it.
21:34ngderf, jesup: you wouldn&#39;t happen to know what Apple&#39;s stance on Opus is currently. I am guessing silence or &quot;AAC is nice&quot;, but would welcome surprising news.
21:35derfng: They&#39;re shipping it in multiple products.
21:35ng!
21:36derfThe browser support is only in their hown-grown CAF container, though.
21:36derf(and WebRTC)
21:36derf*home-grown
21:36ngderf: well that is good first step
21:36derfWasn&#39;t gonna complain.
21:38TD-LinuxI have yet to t