-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Video file names in standard format #70
base: main
Are you sure you want to change the base?
Conversation
…arams for recorder.start (consent boolean, trial type string)
…trial recording extension to recorder.start)
…, test new createFileName method
🦋 Changeset detectedLatest commit: f6b667b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
packages/record/src/trial.ts
Outdated
this.recorder?.start(false, `${this.jsPsych.getCurrentTrial().type}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the type need to be converted to a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, you're right, this is a class name, not the 'trial_type' string 🤦♀️ Fixing it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the current plugin name like this: this.jsPsych.getCurrentTrial().type.info.name
, but for some reason the jsPsych types produce errors in that line (info
property is missing from the jsPsych plugin class interface). So I created a new interface that extends the jsPsych plugin interface.
I doubled checked in the jsPsych code that plugin classes must be created with PluginInfo, and PluginInfo must have a name property (string). Also, when running a trial, the jsPsych code assumes that a trial's pluginInfo
value contains a name
. So if a plugin class doesn't have a value for info.name
, then I don't think it would work.
Thus I don't think we need to do any error catching here for the existence of info.name
on the class, but I'm open to suggestions if you disagree. If so, maybe we could put in a default value for the plugin name, or throw an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the fact that TrialRecordExtension
is not a plugin change anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the fact that this is an extension is the reason why getting the plugin type here is tricky. The TrialRecordExtension
can run over any plugin, and the plugin info isn't automatically accessible from within the extension code. But since we're waiting for the plugin to load before we start recording, we can also ask jsPsych at that point what the current trial type is (this.jsPsych.getCurrentTrial().type
). This gives us the name of the plugin class (e.g. HtmlKeyboardResponsePlugin
), and from that we can use .info.name
to get the plugin/trial type string (e.g. "html-keyboard-response").
…n name from info, fix types, fix/add tests
This PR updates the video file names in the
record
package so that they match the format used by EFP and expected by the lookit-api and and the AWS S3-Lambda trigger.consent-videoStream_{study}_{frame_id}_{response}_{timestamp}_{random_digits}.webm
videoStream_{study}_{frame_id}_{response}_{timestamp}_{random_digits}.webm
This naming system requires two arguments from the plugin that calls
recorder.start
:frame_id
valueThe
frame_id
value is the same one that lookit-jspsych uses in the responsesequence
field, and that's used in the lookit-api to identify trials. The format is:{trial_index}-{trial_type}
. Trial types are not necessarily unique across the experiment, but the trial indices are.Implementation note
Our previous file naming system used a set of pre-defined strings as an argument in the
recorder.start
method:"consent" | "session_video" | "trial_video"
. It would be nice to still have a set of pre-defined strings for the newtrial_type
string passed torecorder.start
. But I don't think that's makes sense now, since our trial recording extension is meant to be used with any jsPsych plugin, so we'd have to maintain a long list of all (currently ~40) jsPsych trial types ("html-button-response", etc.). So this argument is just an unconstrained string. I'm open to suggestions for ways of constraining or validating it.TO DO
As discussed here #36, we would like to give researchers the ability to assign their own unique/meaningful trial IDs, and use these index-type IDs as a fallback. But in the interest of meeting our first MVP, I'm just implementing the index-type solution for now, and we can add the support for researcher-provided trial IDs later.