Feature: Local recording (Ready for review) - #3223
Conversation
|
Hi, thanks for your contribution! |
|
@jitsi-jenkins signed. |
| "js-md5": "0.6.1", | ||
| "jwt-decode": "2.2.0", | ||
| "lib-jitsi-meet": "github:jitsi/lib-jitsi-meet#98acf1336df7a6fe6fc27c16c829ef607ec20ceb", | ||
| "libflac": "git+https://github.com/ztl8702/libflac.git#31368097eaf9dcb5ef59365ef60b259cb7b97f07", |
There was a problem hiding this comment.
Note: this is a temporary solution.
|
Jenkins this one is ok to test. |
87c8255 to
f9f49c6
Compare
| } | ||
| { | ||
| isModerator | ||
| && <div className = 'info-dialog-action-links'> |
There was a problem hiding this comment.
Currently I am just using the CSS rules defined in info-dialog.scss, which is probably not intended for general use outside of InfoDialog.
There was a problem hiding this comment.
Probably not but you can break them out into a separate css file. For example, they're inline dialogs so you can create an inline-dialog.scss and refactor for reuse.
| */ | ||
| export function signalLocalRecordingEngagement(isEngaged: boolean) { | ||
| return { | ||
| type: isEngaged ? LOCAL_RECORDING_ENGAGED : LOCAL_RECORDING_UNENGAGED |
There was a problem hiding this comment.
I generally don't see this pattern in jitsi-meet of dynamically selecting the actionType. Usually each action, with the except of action creators, returns one type. Keep that consistency might be worthwhile unless you feel strongly about it.
| isOpen = { isDialogShown } | ||
| onClose = { this._onCloseDialog } | ||
| position = { 'top right' }> | ||
| <ToolbarButton |
There was a problem hiding this comment.
I think toolbar is at least the one place in the repo where web accessibility is being considered. You should pass in an accessibility label prop.
| this._timer = setInterval( | ||
| () => { | ||
| this.setState((_prevState, props) => { | ||
| const nowTime = new Date(Date.now()); |
There was a problem hiding this comment.
Are you certain you need to pass in Date.now()? Not having worked with Date in over a year, I forget but I think new Date will be initialized with Date.now() automatically.
| const nowTime = new Date(Date.now()); | ||
|
|
||
| return { | ||
| durationString: this._getDuration(nowTime, |
There was a problem hiding this comment.
Am I correct that these stats are only used by the info dialog? If so, do you need to dispatch the stats into the store?
There was a problem hiding this comment.
Oops I can't see which exact line you are referring to. But I assume it is Props.stats?
There was a problem hiding this comment.
Emm.. So I was thinking about perhaps the RecordingController should be detecting the presence of each participant (i.e. their "stats") and firing statsUpdate if there are any changes. That's why there is a Redux action for stats update. However:
a) I am not sure how I should call dispatch in LocalRecording
b) there might not be a real performance gain by detecting the changes in stats, apart from complicating the logic.
So I ended up just polling the stats every second.
| * @returns {void} | ||
| */ | ||
| componentWillUnmount() { | ||
| if (this._timer) { |
There was a problem hiding this comment.
Fun thing about clearInterval/clearTimeout is that they can take in falsy/incorrect values, negating the nee for a truthy check.
There was a problem hiding this comment.
You are right. But flow will complain if I remove the null check:
Cannot call `clearInterval` with `this._timer` bound to `intervalId` because null or undefined [1] is incompatible with
`IntervalID` [2].
| 4: 'FLAC__STREAM_ENCODER_VERIFY_MISMATCH_IN_AUDIO_DATA', | ||
|
|
||
| // One of the callbacks returned | ||
| // a fatal error. |
There was a problem hiding this comment.
So someone might point out to you that you should try to fill out the 80 character line limit as much as you can before line breaking. This applies to all comments.
|
|
||
| // convert to Uint32, | ||
| // appearantly libflac requires 32-bit signed integer input | ||
| // FIXME: why unsigned 32bit array? |
There was a problem hiding this comment.
It'll be worthwhile to go through your comments and answer these questions if possible. My concern at this point is there's one person that jitsi knows of with context on how the internals of local recording works, and that's this PR's author, which is the one and only Radium. Leaving code that others can update and parse readily would be appreciated by the next person that has to work here. That is assuming it's not unreasonable to answer the questions due to various possible constraints.
| } | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
What is this commented out chunk for?
There was a problem hiding this comment.
That was a piece of code for buffering the audio bits that arrived before libflac finishes initialization. Now with the ensureInitialized approach, this is handled in the adapter, so it is not necessary at the flacEncodeWorker level. I will remove it.
| const logger = require('jitsi-meet-logger').getLogger(__filename); | ||
|
|
||
| ReducerRegistry.register('features/local-recording', (state = {}, action) => { | ||
| logger.debug(`Redux state (features/local-recording):\n ${ |
There was a problem hiding this comment.
I recommend removing this logger statement as the reducer can fire many times a second. I assume it'll be really noisy on mobile which during development may log messages onto the screen.
| return { | ||
| ...state, | ||
| isEngaged: true, | ||
| recordingStartedAt: new Date(Date.now()), |
There was a problem hiding this comment.
The idea of a reducer is that it is a pure function, meaning a certain set of arguments will always return a certain result. By introducing Date.now() here that's introducing something less pure, less predictable. Moving it out into action or passing it into the action would be preferred to maintain that pureness.
Mayne even the recordingController could also be passed in, but I'm torn on that.
There was a problem hiding this comment.
ohhh.. Yes it should be passed in as part of the event.
| * Constructor. | ||
| */ | ||
| constructor() { | ||
| super(); |
There was a problem hiding this comment.
React component constructors take in props and props should be passed into the super call.
| constructor() { | ||
| super(); | ||
| this.state = { | ||
| durationString: 'N/A' |
There was a problem hiding this comment.
Technically N/A could go into translation.
| * @returns {number} | ||
| */ | ||
| _getRandomToken() { | ||
| return Math.floor(Math.random() * 10000) + 1; |
There was a problem hiding this comment.
I suppose if you want to do a random token and not key off of some existing value, I wonder if Math.random is random enough. Maybe checking for uniqueness is also warranted or changing this bit to be less likely to conflict with an existing value.
There was a problem hiding this comment.
Well. Just make it larger. Notes to self: https://github.com/jitsi/jitsi-meet/blob/a5538adf8a68270c1cdaceac32487b3521c25d54/react/features/base/util/randomUtil.js
|
|
||
| recordingController.onWarning = function(message) { | ||
| dispatch(showNotification({ | ||
| title: i18next.t('localRecording.localRecording'), |
There was a problem hiding this comment.
Looks like notification might be able to take in a titleKey and descriptionKey, thereby making making it to the middleware and the controller don't have to worry about translation.
|
Jenkins test this please. |
There was a problem hiding this comment.
This 0 will need to be replaced by state['features/base/settings'].micDeviceID.
There was a problem hiding this comment.
If I'm not recording does this empty function make it so I can't close the dialog on outside click?
There was a problem hiding this comment.
Yes.. So the only way to open/close LocalRecordingInfoDialog is by clicking on the toolbar button. I personally found it annoying when the dialog closes when I clicked elsewhere. That's for the convenient of debugging though, and the design should be changed.
There was a problem hiding this comment.
Also take into account the dialog being shown but the toolbar having auto-hidden.
There was a problem hiding this comment.
I forgot that maybe the design will change.
There was a problem hiding this comment.
For consistency, I'd try to get verification on strings whenever verification on design is ever obtained. I don't think in the UI the local participant is referred to as local user
There was a problem hiding this comment.
I see this is set to -1 by default. Will it make sense to see the -1? What does knowing the session token do for the user?
There was a problem hiding this comment.
Do remote users have currentSessionToken defined?
There was a problem hiding this comment.
-
The reason for
-1is that I just want something that is not a valid session id to indicate that there has been no recording sessions, since all session ids are non-negative numbers. Maybe I should make it a const rather than using a magic number. -
Only the moderator generates "new" sessionTokens and sends them with START_COMMAND. Remote users (non-moderators), upon receiving the START_COMMAND, will update their local
currentSessionTokenwith the one received. -
Essentially
currentSessionTokenfunctions like a revision id ofSTART_COMMANDto make sure the states of each user is synced with the moderater's.
There was a problem hiding this comment.
Is it possible for prev to be undefined? Does that look okay in the UI if it's undefined?
There was a problem hiding this comment.
Hmm. Yes. And moment.js will output the string invalid dates which is displayed on the UI.
There was a problem hiding this comment.
Added a null check. 5fc7078#diff-c79e43537b94c65e3956d4a7bd366a52R272
There was a problem hiding this comment.
componentWillMount is deprecated in newer react. I think this logic could be moved to componentDidMount.
There was a problem hiding this comment.
You mentioned test case about a reload. I assume that's the same issue around a new participant joining and potentially missing a start command. Is that correct?
There was a problem hiding this comment.
Yeah. When I register for XMPP events in the middleware, after CONFERENCE_JOINED, the start command (in remote presence) is already missed. Current workaround is let each client send a probe (sendCommandOnce) to force the moderator to resend the command. :(
There was a problem hiding this comment.
Another issue I observed is when the moderator reloads, the command will be removed from the presence. Is there a way to keep START_COMMAND in presence regardless of whether its origin sender is connected? START_COMMAND/STOP_COMMAND should be some kind of "persistent state" that belongs to the room.
Some kind of mechanism like the chat history.
There was a problem hiding this comment.
We've been thinking about the same problem in other situations as well. Let's discuss it when we talk. I think it's OK to merge it this way.
There was a problem hiding this comment.
Maybe the notification should be reworded for the moderator?
|
Jenkins, this one is ok to test. |
04946f3 to
18a4fe5
Compare
There was a problem hiding this comment.
What prevents a non-moderator from starting (or stopping) the recording?
There was a problem hiding this comment.
Nothing technically, it's just the use case that I have pictured: imagine recording an interview, normally I wouldn't want the guests to be messing around with the controls. However, there is indeed a loophole in here, because the "interviewer" might not necessarily be the one with the moderator rights.
There was a problem hiding this comment.
The question is whether or not the "local recording" rights should be aligned with moderator rights.
There was a problem hiding this comment.
Does getUserMedia support an audioBitsPerSecond constraint? I only see it defined for the MediaRecorder API.
There was a problem hiding this comment.
Does the first buffer contain only the header?
There was a problem hiding this comment.
Yes it does.
I find this a little confusing. We have an array of buffers, and the first one is special. It contains the WAV header. The rest contain the PCM in float format. When we export we have to handle the first one in a special way.
Can we keep only PCM in the buffers, and save the header separately (or even better, create it on demand)?
There was a problem hiding this comment.
Yeah. That's what I am thinking about regarding backing up to disk (the IndexedDB thing).
Basically, currently all the RecordingAdapters have the responsibility of both recording and exporting the audio. The currently encoded bits ("buffers") are part of the RecordingAdapter's internal state.
With IndexedDB backups, there needs to be a separation between recording/encoding and exporting, because the buffers to be exported will be in IndexedDB rather than RecordingAdapters themselves.
fix Promise in setMuted
move duplicate code from WavAdapter and FlacAdapter to a base class
f594fb0 to
df6df1c
Compare
|
Is this feature online at http://meet.jit.si/ ? |
|
Not yet.
From: <noreply@github.com> on behalf of Robert Riemann <notifications@github.com>
Reply-To: jitsi/jitsi-meet <reply@reply.github.com>
Date: Thursday, 21 February 2019 at 8:41 am
To: jitsi/jitsi-meet <jitsi-meet@noreply.github.com>
Cc: Z Radium <ztl8702@126.com>, Author <author@noreply.github.com>
Subject: Re: [jitsi/jitsi-meet] Feature: Local recording (Ready for review) (#3223)
Is this feature online at http://meet.jit.si/ ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Hi, can you give some input on how to go about mixing the recordings of all the users? Say the moderator has a 60 minute continuous audio recording, but the second speaker spoke only for 10 minutes somewhere between those 60 minutes and his recording file is 10 minutes long. How will I know where to plug in those 10 minutes of the second speaker's recording in my 60 minutes recording. |
8 participants
Overview
The basic idea is to obtain uncompressed, high-quality audio locally, which will not suffer from jittering or temporary network loss. For more background information (such as the use cases), see #2648 and the GSoC proposal here.
Update 08/27: for those interested, take a look at my blog post.
Note: This PR is part of Google Summer of Code 2018
How to enable
This feature is disabled by default as the user consent UI and there are security concerns regarding modified clients.
To enable it on your Jitsi Meet instance, add the following to your config.js:
Demo instance
An instance of Jitsi Meet with local recording enabled is available at https://radium2.jitsi.net.
To try out this feature, use two browsers to join the same room on https://radium2.jitsi.net, and in the moderator's browser press key "L" to bring up Local Recording Controls. (No UI button is enabled as per UX requirements).
What is included
Current Limitation
muc_allownersdisabled), otherwise undefined behaviour can happen when two different users try to start/stop local recording.Future work (in follow up PRs)