Skip to content
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

Fix video recording playback issues #127

Merged
merged 5 commits into from
Jan 30, 2025
Merged

Conversation

becky-gilbert
Copy link
Contributor

Fixes #126

This fixes a problem with video recordings not playing in some browsers. The problem is due to initializing a recorder without specifying the codecs (we had the same thing happen in EFP: lookit/ember-lookit-frameplayer#365). To fix this, we just need to specify the mimeType codecs value as part of the recording options when the recorder is initialized. This PR does the following:

  • In the video config plugin, adds a getCompatibleMimeType method to check mime type compatibility and get the first compatible type from the list. This mime type string must be specified when the plugin initializes the jsPsych recorder via jsPsych.pluginAPI.initializeCameraRecorder.
  • In the recorder class, gets the mime type value from the existing recorder and stores it. This is needed in case the recorder class re-initializes the jsPsych recorder (via its initializeRecorder method and jsPsych.pluginAPI.initializeCameraRecorder).
  • Sets a default/fallback mime type of "video/webm".
  • Adds tests.

…ng jsPsych recorder initialization, and get mime type value from jsPsych recorder when a new recorder is set up
Copy link

changeset-bot bot commented Jan 16, 2025

🦋 Changeset detected

Latest commit: 0e6de08

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lookit/record Patch

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

@becky-gilbert becky-gilbert self-assigned this Jan 16, 2025
@becky-gilbert becky-gilbert requested a review from mekline January 16, 2025 23:27
Copy link
Collaborator

@okaycj okaycj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am misunderstanding something. Is the issue here that a video recorded on someone else's browser isn't viewable on their browser?

@becky-gilbert
Copy link
Contributor Author

I think I am misunderstanding something. Is the issue here that a video recorded on someone else's browser isn't viewable on their browser?

@okaycj sorry for not being clear. The problem is that, when the media recorder is initialized in Chrome and the mime type is left unspecified, Chrome sets it to “video/x-matroska;codecs=avc1,opus”. When this happens, Firefox is unable to play the file (mime type not supported error). The same thing happens when the recorder is initialized with "video/webm" as the mime type, but without specific codecs. This is as much as I understand about the problem right now. I did find a few posts that might be relevant:

https://stackoverflow.com/questions/64233494/mediarecorder-does-not-produce-a-valid-webm-file
https://issues.chromium.org/issues/40126508

I've tested this and found that videos recorded in Chrome can be played in Firefox as long as you also specify the container with a compatible codec (e.g. "video/webm;codecs=vp9,opus"). So my proposed solution is to put together a list of all complete container/codec strings that might be supported on any browsers that we officially support1, and then when a recorder is set up during the experiment, iterate through that list to find one that is supported by the participant's browser and pass the value to the media recorder initialization function. (This is currently how we do things in EFP, so if we decide to make changes here then we may want to make them in EFP as well.)

Footnotes

  1. Right now we only officially support desktop/laptop Chrome and Firefox. But if we want to support experiments on mobile devices, we will need to expand this list to include mobile browsers, which may mean coming back to this issue.

@becky-gilbert becky-gilbert requested a review from okaycj January 30, 2025 18:59
@becky-gilbert becky-gilbert merged commit 9fe8a0c into develop Jan 30, 2025
2 checks passed
@becky-gilbert becky-gilbert deleted the fix-recording-codecs branch January 30, 2025 19:05
@jodeleeuw
Copy link

Just happened to see this issue go by on my GH feed. Is this something we should fix upstream?

@becky-gilbert
Copy link
Contributor Author

becky-gilbert commented Jan 30, 2025

Hey @jodeleeuw! I think it'd be a good idea for jsPsych to use a sensible default value for the media recorder options parameter, since leaving the mime type string empty (or specifying a container without specifying codecs) seems to cause problems. Of course you can still allow this value to be passed in by the user.

We just support Chrome and Firefox, so it's pretty easy to cover these browsers (back to very early versions) with just two mime type strings:

const mime_types = [
  "video/webm;codecs=vp9,opus",
  "video/webm;codecs=vp8,opus",
];

I think the downside is that you have to update the list as browsers change or you want to support a broader range of browsers. But again, the user can always supply their own value if this list is too narrow or becomes outdated. I made a note about this in our code:

Note: we will likely need to continuously update the mime_types list as new formats become supported, we support other browsers/versions, etc.

I'd be happy to put in a PR with the change and corresponding docs, if this sounds like a good idea to you. If so, let me know if you want to use our same mime types list, a single value, or something else.

@becky-gilbert
Copy link
Contributor Author

becky-gilbert commented Jan 30, 2025

Ah, so I was going to propose this change in initializeCameraRecorder, but I just noticed that you already have a mime_type parameter for your camera initialization plugin (we're not using that plugin, but we are using initializeCameraRecorder from the plugin API). So in this case the change is super easy - just put in a default value instead of null, and/or add a warning in the docs about leaving this blank or using "video/webm" without a codecs value.

@jodeleeuw
Copy link

Ah, great - thanks for looking into this! I'll open an issue that points to this conversation. Feel free to PR if you'd like, but I can also ask someone on the team to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video playback issues
5 participants