Announce the correct MIME type for video sources - #92
Merged
dillonbbailey merged 1 commit intoSep 16, 2026
Merged
Conversation
The video handler accepted .mp4, .webm and .ogg but hardcoded type="video/webm" on every <source> it emitted, so the nine MP4s in the curriculum were served announcing a format they are not. The type attribute is what a browser consults to decide whether to fetch a source at all. A browser that cannot play the announced format skips the source without downloading it, and since there is only one source the reader gets the "your browser does not support the video tag" fallback instead. Browsers with WebM support fetch the file anyway and sniff the real container, which is why this has been invisible; a browser without WebM support drops an MP4 it could have played natively. Map the extension to its MIME type instead. Also stop dropping the element when the URI is not in builder.images. The inner branch had no else, and because control had already entered the outer branch super().visit_image was never reached either, so such a node emitted no markup, no fallback text and no warning. External video URLs are not collected into builder.images, so they hit exactly this path. Emit the element either way and rewrite the URI only when the builder knows it. Verified on a clean build: the nine .mp4 sources now announce type="video/mp4" and the thirty-four .webm sources are unchanged. Warning set is byte-identical to main at 163; suite passes at 140. Addresses NVIDIA-Omniverse#91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dillon Bailey <dillonb@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #91
The bug
LOUSDHTMLTranslatorMixin.visit_imageaccepts.mp4,.webmand.ogg, then hardcodes a single MIME type for all of them:So the curriculum's MP4s shipped announcing a format they are not:
typeis what a browser consults to decide whether to fetch a source at all. A browser that cannot play the announced format skips it without downloading, and there is no second<source>— so the reader gets the fallback text. Chrome/Firefox/Edge support WebM, accept the hint, fetch the file and sniff the real container, which is why this has gone unnoticed. A browser without WebM support drops an MP4 it could have played natively (Safari only gained WebM in 14.1; older iOS Safari has no VP9).Also fixed
When the URI is not in
builder.imagesthe inner block is skipped, and since control already entered the outer branchsuper().visit_image(node)never runs either. Such a node emitted no markup, no fallback text and no warning — the video silently vanished. External video URLs are not collected intobuilder.images, so they hit exactly this path.The element is now emitted either way, with the URI rewritten only when the builder knows about it.
Verification
Clean build, before vs after:
.mp4sourcesvideo/webm✗video/mp4✓.webmsourcesvideo/webmvideo/webm(unchanged)9 MP4 and 34 WebM
<source>elements in the built HTML, all now correctly typed. Warning set byte-identical tomainat 163; suite passes at 140.Note
I hit a transient
failed to reach any of the inventorieswarning on one build — an intersphinx network fetch, unrelated to this change. It did not reproduce on rebuild and the warning set matchesmainexactly.