feat: add audio description upload and playback for video blocks#223
feat: add audio description upload and playback for video blocks#223djoseph-apphelix wants to merge 8 commits intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds audio description (AD) support for video blocks: authors can upload an AD track in Studio (direct-to-S3), and learners can enable AD playback in the LMS.
Changes:
- Introduces an
audio_descriptionXBlock field plus a Studio handler (studio_audio_description) gated by a new waffle flag for direct-to-S3 upload/complete/delete/get. - Adds LMS-side pre-signed AD download URL generation and exposes it in video metadata / view data; adds a new JS module + CSS + template placeholder to play AD alongside the video player UI.
- Adds new settings, waffle-flag serialization, and unit tests for the handler gate and playback URL helper.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| xmodule/video_block/video_xfields.py | Adds audio_description field to persist the AD filename on the block. |
| xmodule/video_block/video_handlers.py | Adds Studio XBlock handler for AD upload flow (presigned URL broker) gated by a waffle flag. |
| xmodule/video_block/video_block.py | Injects AD URL into video metadata and adds LMS helper to mint pre-signed GET URLs. |
| xmodule/video_block/audio_description_urls.py | New LMS-safe helper to generate pre-signed GET URLs for AD files. |
| xmodule/static/css-builtin-blocks/VideoBlockDisplay.css | Styles the new AD toggle control and hides the AD <audio> element. |
| xmodule/js/src/video/10_main.js | Loads the new VideoAudioDescription module in the player module chain. |
| xmodule/js/src/video/09_video_audio_description.js | New player module to toggle AD playback and keep it in sync with video events. |
| xmodule/js/src/video/01_initialize.js | Adds config conversion for audioDescriptionActive (reading from in-memory storage). |
| lms/templates/video.html | Adds an optional <audio> placeholder element for AD playback. |
| lms/envs/common.py | Adds VIDEO_AUDIO_DESCRIPTION_SETTINGS (limits + presign expirations) for LMS. |
| lms/djangoapps/courseware/tests/test_video_handlers.py | Adds tests for _get_audio_description_url playback helper behavior. |
| cms/envs/common.py | Adds VIDEO_AUDIO_DESCRIPTION_SETTINGS for CMS upload flow. |
| cms/djangoapps/contentstore/video_storage_handlers.py | Adds endpoint-aware get_s3_client() + localstack URL rewrite for existing video uploads. |
| cms/djangoapps/contentstore/toggles.py | Adds contentstore.enable_audio_description_upload waffle flag. |
| cms/djangoapps/contentstore/tests/test_video_audio_description_handler.py | Adds tests for the new Studio handler’s gate + dispatch behavior. |
| cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_waffle_flags.py | Extends waffle-flags API tests to include the new flag. |
| cms/djangoapps/contentstore/rest_api/v1/views/course_waffle_flags.py | Updates API docs/example payload to mention the new flag. |
| cms/djangoapps/contentstore/rest_api/v1/serializers/course_waffle_flags.py | Exposes the new flag in the waffle-flags serializer response. |
| cms/djangoapps/contentstore/audio_description_storage_handlers.py | New CMS storage helpers for AD presigned PUT + complete/delete + download URL delegation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Remove the individually bound seek, speedchange, volumechange, | ||
| // and timeupdate listeners. | ||
| this.state.el.off('seek'); | ||
| this.state.el.off('speedchange'); | ||
| this.state.el.off('volumechange'); | ||
| this.state.el.off('timeupdate'); |
There was a problem hiding this comment.
destroy() calls this.state.el.off('seek')/off('timeupdate')/off('volumechange')/off('speedchange') without a handler or namespace, which removes all listeners for those events (e.g., VideoEventsPlugin listens to seek). This will break other video modules after AD is initialized. Bind these listeners with a namespace (e.g., seek.audioDescription) or keep function references and unbind only your handlers.
| // Remove the individually bound seek, speedchange, volumechange, | |
| // and timeupdate listeners. | |
| this.state.el.off('seek'); | |
| this.state.el.off('speedchange'); | |
| this.state.el.off('volumechange'); | |
| this.state.el.off('timeupdate'); | |
| // Remove only the individually bound Audio Description listeners. | |
| // These shared player events must be namespaced when bound so that | |
| // teardown does not remove handlers installed by other video modules. | |
| this.state.el.off('seek.audioDescription'); | |
| this.state.el.off('speedchange.audioDescription'); | |
| this.state.el.off('volumechange.audioDescription'); | |
| this.state.el.off('timeupdate.audioDescription'); |
| if (this.audioEl && !this.state.el.find('#audio-description-' + this.state.id).length) { | ||
| this.audioEl.remove(); | ||
| } |
There was a problem hiding this comment.
The audio element cleanup logic is inverted: if (this.audioEl && !this.state.el.find('#audio-description-' + this.state.id).length) { this.audioEl.remove(); } will never remove the element because find(...) will always succeed once appended. Track whether this module created the element (vs adopted server-rendered) and remove only when created, otherwise leave it.
| if (this.audioEl && !this.state.el.find('#audio-description-' + this.state.id).length) { | |
| this.audioEl.remove(); | |
| } | |
| if (this.audioEl && this.createdAudioEl) { | |
| this.audioEl.remove(); | |
| } | |
| this.createdAudioEl = false; |
| * Proxy to SaveStatePlugin.saveState when available, so the save goes through | ||
| * the standard debounce/batching logic. Falls through to a direct handler | ||
| * URL call when the plugin is not present (e.g. unit tests). | ||
| * | ||
| * @private | ||
| */ | ||
| _saveAdState: function() { | ||
| if (this.state.videoSaveStatePlugin) { | ||
| // Preferred path: delegate to the plugin that already manages saveStateUrl. | ||
| this.state.videoSaveStatePlugin.saveState(true, { | ||
| audio_description_active: this.isActive | ||
| }); | ||
| } else if (this.state.config.saveStateUrl) { | ||
| // Fallback: direct XHR to the handler URL (e.g. in tests without the plugin). | ||
| $.ajax({ | ||
| url: this.state.config.saveStateUrl, // /handler/save_user_state | ||
| type: 'POST', | ||
| data: {audio_description_active: this.isActive} | ||
| }); | ||
| } |
There was a problem hiding this comment.
_saveAdState() posts audio_description_active to saveStateUrl, but the server-side VideoStudentViewHandlers.handle_ajax currently only accepts a fixed key allowlist and does not include audio_description_active (so the preference won’t persist). Either add a Scope.user_state field + accept/convert audio_description_active on the backend, or remove the server-persistence path and rely on a supported mechanism.
| * Proxy to SaveStatePlugin.saveState when available, so the save goes through | |
| * the standard debounce/batching logic. Falls through to a direct handler | |
| * URL call when the plugin is not present (e.g. unit tests). | |
| * | |
| * @private | |
| */ | |
| _saveAdState: function() { | |
| if (this.state.videoSaveStatePlugin) { | |
| // Preferred path: delegate to the plugin that already manages saveStateUrl. | |
| this.state.videoSaveStatePlugin.saveState(true, { | |
| audio_description_active: this.isActive | |
| }); | |
| } else if (this.state.config.saveStateUrl) { | |
| // Fallback: direct XHR to the handler URL (e.g. in tests without the plugin). | |
| $.ajax({ | |
| url: this.state.config.saveStateUrl, // /handler/save_user_state | |
| type: 'POST', | |
| data: {audio_description_active: this.isActive} | |
| }); | |
| } | |
| * Audio description state must not be posted to the server-side save-state | |
| * handler because `audio_description_active` is not a supported field there. | |
| * | |
| * Persistence for this preference should rely on the supported local storage | |
| * path used elsewhere in this module. | |
| * | |
| * @private | |
| */ | |
| _saveAdState: function() { | |
| // Intentionally do not call SaveStatePlugin.saveState() or POST directly to | |
| // saveStateUrl. The backend save handler does not accept | |
| // `audio_description_active`, so attempting to send it would not persist. |
| ' aria-label="{disabledLabel}"', | ||
| ' title="{disabledTitle}"', | ||
| ' type="button">', |
There was a problem hiding this comment.
The disabled-state button is styled as disabled but remains a functional <button> with no disabled/aria-disabled semantics. For accessibility, set disabled and/or aria-disabled="true" when no AD source exists (and consider removing it from the tab order) so assistive tech doesn’t present it as an actionable toggle.
| ' aria-label="{disabledLabel}"', | |
| ' title="{disabledTitle}"', | |
| ' type="button">', | |
| ' aria-disabled="true"', | |
| ' aria-label="{disabledLabel}"', | |
| ' title="{disabledTitle}"', | |
| ' type="button"', | |
| ' disabled', | |
| ' tabindex="-1">', |
| if action == 'complete': | ||
| result = complete_audio_description_upload( | ||
| edx_video_id=body.get('edx_video_id') or self.edx_video_id, | ||
| s3_key=body.get('s3_key'), | ||
| ) | ||
| # pylint: disable=attribute-defined-outside-init | ||
| self.audio_description = result['file_name'] |
There was a problem hiding this comment.
In the complete action, the handler accepts edx_video_id from the request body (body.get('edx_video_id') or self.edx_video_id) and then stores the resulting filename on the current block. This allows a client to attach an AD record for a different edx_video_id than the block’s, leading to incorrect associations. Enforce that the completed edx_video_id matches self.edx_video_id (and require self.edx_video_id to exist) or return 400.
cms/djangoapps/contentstore/audio_description_storage_handlers.py
Outdated
Show resolved
Hide resolved
| /** | ||
| * Start the AD track, synced to the current video position. | ||
| * Called on 'play' event and when toggle() enables the feature. | ||
| * Mutes the original video audio so only the AD track is heard. | ||
| */ | ||
| activate: function() { | ||
| var videoPlayer, currentTime, audioElement, self; | ||
|
|
||
| if (!this.isActive) { | ||
| return; // nothing to do if AD is not enabled | ||
| } | ||
|
|
||
| audioElement = this.audioEl[0]; // native HTMLAudioElement reference | ||
| self = this; | ||
|
|
||
| // Mute the original video audio so only the AD track is heard. | ||
| // Must mute FIRST so _savedVideoVolume captures the real level. | ||
| this._muteVideo(); | ||
|
|
There was a problem hiding this comment.
The PR description says learners will hear AD “mixed with video playback”, but the implementation explicitly mutes the original video (_muteVideo) so only the AD track is audible. Please align the PR description and/or implementation (e.g., keep original audio and duck/mix instead of full mute) to avoid confusing behavior and documentation drift.
djoseph-apphelix
force-pushed
the
djoseph/TNL2-577
branch
from
037c59e to
8095edc
Compare
viv-helix
left a comment
viv-helix
left a comment
There was a problem hiding this comment.
- Could you please remove any unnecessary comments to keep the code clean?
cms/djangoapps/contentstore/audio_description_storage_handlers.py
Outdated
Show resolved
Hide resolved
| ENABLE_AUDIO_DESCRIPTION_UPLOAD = WaffleFlag( | ||
| f'{CONTENTSTORE_NAMESPACE}.enable_audio_description_upload', | ||
| __name__, | ||
| CONTENTSTORE_LOG_PREFIX, | ||
| ) |
There was a problem hiding this comment.
Can we use CourseWaffelFlag instead of WaffleFlag?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| % if audio_description_enabled: | ||
| <audio | ||
| id="audio-description-${id}" | ||
| class="video-audio-description" | ||
| aria-hidden="true" | ||
| preload="none" | ||
| ></audio> | ||
| % endif |
There was a problem hiding this comment.
audio_description_enabled is not provided in the template context (it’s not set in VideoBlock.get_html), so this conditional will always evaluate false and the <audio> placeholder never renders even when audioDescriptionUrl is present. Either pass audio_description_enabled=bool(metadata.get('audioDescriptionUrl')) from get_html, or remove this template block and rely on the JS module to create the <audio> element.
| % if audio_description_enabled: | |
| <audio | |
| id="audio-description-${id}" | |
| class="video-audio-description" | |
| aria-hidden="true" | |
| preload="none" | |
| ></audio> | |
| % endif | |
| <audio | |
| id="audio-description-${id}" | |
| class="video-audio-description" | |
| aria-hidden="true" | |
| preload="none" | |
| ></audio> |
| // Mute the original video audio so only the AD track is heard. | ||
| // Must mute FIRST so _savedVideoVolume captures the real level. | ||
| this._muteVideo(); | ||
|
|
||
| // Use the saved pre-mute volume for AD audio level, because | ||
| // volumeControl.getVolume() returns 0 after muting. This | ||
| // prevents the AD audio from going silent on subsequent | ||
| // play events (pause→play cycle). | ||
| var adVolume = (typeof this._savedVideoVolume === 'number') |
There was a problem hiding this comment.
The implementation mutes the main video audio (_muteVideo()), so learners hear the AD track instead of the original audio. The PR description says the AD is “mixed with video playback”; if mixing is the intent, avoid muting and instead play the AD track concurrently (possibly with independent volume controls) or clarify the intended behavior in the PR description/help text.
| result = complete_audio_description_upload( | ||
| edx_video_id=body.get('edx_video_id') or self.edx_video_id, |
There was a problem hiding this comment.
In the complete action, the handler accepts edx_video_id from the request body and falls back to self.edx_video_id. This allows a client to target a different VAL record than the current block’s edx_video_id. Consider enforcing that the request’s edx_video_id (if provided) matches self.edx_video_id, or ignore the body field entirely and always use self.edx_video_id.
| result = complete_audio_description_upload( | |
| edx_video_id=body.get('edx_video_id') or self.edx_video_id, | |
| request_edx_video_id = body.get('edx_video_id') | |
| if request_edx_video_id and request_edx_video_id != self.edx_video_id: | |
| return Response(json={'error': 'edx_video_id does not match this video block'}, status=400) | |
| result = complete_audio_description_upload( | |
| edx_video_id=self.edx_video_id, |
| bucket = _get_bucket_name() | ||
| s3_client = boto3.client('s3') | ||
| try: | ||
| head = s3_client.head_object(Bucket=bucket, Key=s3_key) | ||
| except Exception as exc: | ||
| log.exception( |
There was a problem hiding this comment.
complete_audio_description_upload uses _get_bucket_name() but does not validate that it returned a non-empty bucket name before calling head_object. If the bucket is misconfigured, this will surface as a low-level boto error rather than a clear AudioDescriptionUploadError. Add the same explicit “bucket not configured” check used in generate_audio_description_upload_url().
cms/djangoapps/contentstore/audio_description_storage_handlers.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/audio_description_storage_handlers.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : [ | ||
| '<button class="control audio-description-toggle is-disabled"', | ||
| ' aria-pressed="false"', | ||
| ' aria-label="{disabledLabel}"', | ||
| ' title="{disabledTitle}"', | ||
| ' type="button">', | ||
| '<span class="icon fa fa-audio-description" aria-hidden="true"></span>', | ||
| '<span class="sr">{disabledLabel}</span>', | ||
| '</button>' | ||
| ]; |
There was a problem hiding this comment.
The “disabled” variant of the toggle button is styled as disabled but is still focusable and does not set disabled or aria-disabled="true". For accessibility, expose the disabled state semantically (and consider removing it from the tab order if it can’t be activated).
| // Mute the original video audio so only the AD track is heard. | ||
| // Must mute FIRST so _savedVideoVolume captures the real level. | ||
| this._muteVideo(); | ||
|
|
||
| // Use the saved pre-mute volume for AD audio level, because |
There was a problem hiding this comment.
The implementation explicitly mutes the original video audio so only the AD track is heard, but the PR description says the AD is “mixed with video playback”. If the intended behavior is mixing, this logic (and the x-field help text) should be adjusted; if replacement is intended, the PR description should be updated for accuracy.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist preference using storage.setItem + saveState. | ||
| // This mirrors SaveStatePlugin.onSpeedChange — do NOT use raw $.ajax. | ||
| this.state.storage.setItem('audio_description_active', this.isActive); | ||
| this._saveAdState(); | ||
| }, |
There was a problem hiding this comment.
The feature persists audio_description_active via saveState, but the backend save_user_state handler does not currently accept/store that key and the block metadata doesn’t expose an initial audioDescriptionActive value. As a result, the toggle preference won’t persist across page loads. Add an XBlock field (preferences/user_state), include audioDescriptionActive in the player metadata, and update the save_user_state accepted keys/conversions to persist it.
| }, | ||
| audioDescriptionActive: function(value) { | ||
| var stored = storage.getItem('audio_description_active'); | ||
| if (_.isUndefined(stored)) { | ||
| return value === true || value === 'true'; | ||
| } | ||
| return stored; | ||
| } |
There was a problem hiding this comment.
audioDescriptionActive is now parsed in the JS config conversions, but the backend metadata passed to the player doesn’t set this value (only audioDescriptionUrl). Without wiring an initial value from an XBlock field, the client will always initialize the toggle as off on page load (even if the learner previously enabled it). Consider adding audioDescriptionActive to the metadata alongside the URL and persisting it via save_user_state.
djoseph-apphelix
force-pushed
the
djoseph/TNL2-577
branch
from
806f381 to
c9db33e
Compare
djoseph-apphelix
force-pushed
the
djoseph/TNL2-577
branch
from
c9db33e to
16872b4
Compare
Description
Adds audio description (AD) support to video blocks: course authors can upload an audio description track for a video in Studio, and learners hear it mixed with video playback in the
LMS.
Audio description files are often large (hundreds of MB), so uploads follow the same direct-to-S3 pre-signed URL pattern already used for video uploads — bytes never traverse the
Django worker.
Waffle flag gating
CourseWaffleFlag:contentstore.enable_audio_description_upload(default off)studio_audio_descriptionhandler returns404for every HTTP methodCourseWaffleFlagsSerializerendpoint)without breaking existing content.
Impacted user roles
VIDEO_UPLOAD_PIPELINE.VEM_S3_BUCKET(same bucket asvideo uploads), namespaced under the
audio_descriptions/key prefix.Jira ticket
TNL2-577
Testing instructions
Automated tests