Restore including videos in epubs (BL-16051)#7770
Restore including videos in epubs (BL-16051)#7770JohnThomson wants to merge 1 commit intoVersion6.3from
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| includeFilesNeededForBloomPlayer: false, // no activities in Epubs. | ||
| includeVideos: true, |
There was a problem hiding this comment.
🚩 Videos are now processed twice for ePUB publishing
With includeVideos: true, video files are now copied into the staging folder. Since processVideos defaults to true, SignLanguageApi.ProcessVideos runs in MakeDeviceXmatterTempBook (src/BloomExe/Publish/PublishHelper.cs:1041-1050) with videoControls: false. Later, EpubMaker's CopyVideos (src/BloomExe/Publish/Epub/EpubMaker.cs:2225-2247) calls PrepareVideoForPublishing again with videoControls: true. This double processing is functionally correct — the second pass won't re-trim (timings already stripped) and correctly adds the controls attribute — but it's wasteful. The EpubMaker could pass processVideos: false to MakeDeviceXmatterTempBook to avoid the redundant first pass, matching the comment at src/BloomExe/web/controllers/SignLanguageApi.cs:697-698 which explicitly notes that "EpubMaker has different requirements and uses a slightly different process."
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Devin is probably right, but I'm looking for the minimal, safest fix that we can cherry-pick back to 6.2. It might be worth a follow-up task to make this optimization, but since no one has complained about our inability to create epubs with video for about four Bloom versions in which it didn't work at all, optimization feels premature, to say the least. My recommendation would be not do it until someone asks, and definitely not in this PR.
| _omittedPageLabels, | ||
| includeVideoAndActivities: false, // no activities in Epubs. | ||
| includeFilesNeededForBloomPlayer: false, // no activities in Epubs. | ||
| includeVideos: true, |
There was a problem hiding this comment.
🚩 Pre-existing mp4 MIME type becomes more relevant
The GetMediaType method at src/BloomExe/Publish/Epub/EpubMaker.cs:3752-3753 returns "audio/mp4" for .mp4 files. Previously this was less impactful because WantVideo was false for ePUB, meaning video files were excluded from the staging folder and CopyVideos would skip them. Now that videos are included, this MIME type will be used in the ePUB manifest for actual video files. The existing REVIEW comment acknowledges this, but it may warrant revisiting now that videos are actively included in ePUBs. ePUB 3 does list video/mp4 as a valid foreign media type.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not sure what to do about this. Our preview epub reader works fine with audio/mp4. Presumably, a good many other readers do, too, or we would have gotten complaints; this used to work. (Then again, we haven't gotten any complaints from video not working in epubs for about four successive versions of Bloom.) The epub 3 spec at https://www.w3.org/TR/epub/#sec-core-media-types does not list this as a standard mime type (it doesn't have ANY video mime types). However, Devin is right that there is some provision for using a 'foreign' mime type, and another AI suggested that video/mp4 is the best choice.
OTOH, the standard also says that when we use a foreign mime type we should have a fall-back resource that uses a standard one. AI suggested that to be properly conformant we should have a fall-back image to show if the reader can't do video. I consider it at least moderately likely that just changing to video/mp4 will produce errors from epub-3 validators like our Daisy one.
This might be worth researching further, though maybe only given evidence that anyone wants to publish epubs containing video; but I am not inclined to just change it as part of this bug fix.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions.
| _omittedPageLabels, | ||
| includeVideoAndActivities: false, // no activities in Epubs. | ||
| includeFilesNeededForBloomPlayer: false, // no activities in Epubs. | ||
| includeVideos: true, |
There was a problem hiding this comment.
Not sure what to do about this. Our preview epub reader works fine with audio/mp4. Presumably, a good many other readers do, too, or we would have gotten complaints; this used to work. (Then again, we haven't gotten any complaints from video not working in epubs for about four successive versions of Bloom.) The epub 3 spec at https://www.w3.org/TR/epub/#sec-core-media-types does not list this as a standard mime type (it doesn't have ANY video mime types). However, Devin is right that there is some provision for using a 'foreign' mime type, and another AI suggested that video/mp4 is the best choice.
OTOH, the standard also says that when we use a foreign mime type we should have a fall-back resource that uses a standard one. AI suggested that to be properly conformant we should have a fall-back image to show if the reader can't do video. I consider it at least moderately likely that just changing to video/mp4 will produce errors from epub-3 validators like our Daisy one.
This might be worth researching further, though maybe only given evidence that anyone wants to publish epubs containing video; but I am not inclined to just change it as part of this bug fix.
| includeFilesNeededForBloomPlayer: false, // no activities in Epubs. | ||
| includeVideos: true, |
There was a problem hiding this comment.
Devin is probably right, but I'm looking for the minimal, safest fix that we can cherry-pick back to 6.2. It might be worth a follow-up task to make this optimization, but since no one has complained about our inability to create epubs with video for about four Bloom versions in which it didn't work at all, optimization feels premature, to say the least. My recommendation would be not do it until someone asks, and definitely not in this PR.
1 participant
This change is