Allow seeking throughout video files #8844

well-noted · 2024-12-22T20:07:40Z

Parser which allows buffering and scrubbing through video files

github-actions · 2024-12-22T20:07:50Z

Confirmed: well-noted has already signed the Contributor License Agreement (see contributing.md)

netlify · 2024-12-22T20:07:58Z

✅ Deploy Preview for tiddlywiki-previews ready!

Name	Link
🔨 Latest commit	`a53fe9d`
🔍 Latest deploy log	https://app.netlify.com/sites/tiddlywiki-previews/deploys/677ac814055fc50008908948
😎 Deploy Preview	https://deploy-preview-8844--tiddlywiki-previews.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2024-12-22T20:53:51Z

Confirmed: well-noted has already signed the Contributor License Agreement (see contributing.md)

…ut error

arunnbabu81 · 2024-12-23T02:47:14Z

@well-noted would you please tell what changes are made to the video parser in this pull request that are relevant to an end user. I am interested in knowing because I have a video annotation system based on wikitext as mentioned in this talk tiddlywiki post and share demo

pmario

I think there is something conceptually wrong with this PR. Parsers should not manipulate the DOM. --

pmario · 2024-12-23T08:18:04Z

core/modules/parsers/videoparser.js

+	}
+
+	if ($tw.browser) {
+		const processedVideos = new WeakMap();


@Jermolene -- Do we allow WeakMap ES6 (ES2015) and const, async and arrow => functions?

pmario · 2024-12-23T08:19:02Z

core/modules/parsers/videoparser.js


 /*jslint node: true, browser: true */
 /*global $tw: false */
 "use strict";

-var VideoParser = function(type,text,options) {
+var VideoParser = function (type, text, options) {


no spaces between params

pmario · 2024-12-23T08:19:55Z

core/modules/parsers/videoparser.js

@@ -6,29 +6,161 @@ module-type: parser
 The video parser parses a video tiddler into an embeddable HTML element

 \*/
-(function(){
+(function () {


pmario · 2024-12-23T08:22:28Z

core/modules/parsers/videoparser.js

+						}, { passive: true });
+
+						// Cleanup
+						const observer = new MutationObserver(function(mutations) {


no const no let atm

pmario · 2024-12-23T08:23:03Z

core/modules/parsers/videoparser.js

+						// Cleanup
+						const observer = new MutationObserver(function(mutations) {
+							mutations.forEach(function(mutation) {
+								if ([...mutation.removedNodes].includes(video)) {


no spread atm

pmario · 2024-12-23T08:23:43Z

core/modules/parsers/videoparser.js

+
+						// Cleanup
+						const observer = new MutationObserver(function(mutations) {
+							mutations.forEach(function(mutation) {


We usually use $tw.utils.each()

pmario · 2024-12-23T08:24:05Z

core/modules/parsers/videoparser.js

+
+				xhr.send();
+
+				video.addEventListener('loadedmetadata', async () => {


no async atm

pmario · 2024-12-23T08:26:30Z

core/modules/parsers/videoparser.js

+				if (processedVideos.has(video)) return;
+
+				// Create loading overlay
+				const overlay = document.createElement('div');


We do use double quotes "div" for strings

pmario · 2024-12-23T08:28:19Z

core/modules/parsers/videoparser.js

+				// Create loading overlay
+				const overlay = document.createElement('div');
+				overlay.style.cssText = 'position:absolute;top:0;left:0;width:100%;height:100%;background:rgba(0,0,0,0.5);display:flex;align-items:center;justify-content:center;color:white;';
+				overlay.innerHTML = 'Loading 0%';


hmmm, innerHTML -- I am not sure if a "parser" should modify the DOM structure. Ususally widgets need to be used to create the DOM

buggyj · 2025-01-04T14:08:01Z

@well-noted do you have an example showing what functionality this adds?

well-noted · 2025-01-04T17:27:43Z

I think there is something conceptually wrong with this PR. Parsers should not manipulate the DOM. --

Having sat with this for some time, it does occur to me that some of these features are likely outside the realm of a "parser" and might be better suited for a widget.

well-noted · 2025-01-04T17:34:03Z

@well-noted do you have an example showing what functionality this adds?

https://talk.tiddlywiki.org/t/playing-with-parsers/11518/9?u=well-noted

well-noted · 2025-01-05T17:59:50Z

Added new commit that improves upon original commit and also in version previewed yesterday:

Known limitation, timestamps do not restore on mobile, probably something to do with buffering/loading times

well-noted · 2025-01-05T18:05:54Z

Additional note, one will need to transclude the src tiddler into another tiddler for this to work, because if one is viewing the video within the src, then, when the timestamp updates, the src is temporarily interrupted -- if this were to merged, therefore, we would need to make additional changes so that, if the current tiddler is the source tiddler, it would not store timestamps.

pmario · 2025-01-05T18:22:43Z

core/modules/parsers/videoparser.js

+(function () {
+
+	/*jslint node: true, browser: true */
+	/*global $tw: false */


indentation problem. It is safe to remove the (function(){ wrapper. So your "prettyfier" does not create redundant indents.

Jermolene · 2025-01-06T11:27:32Z

Hi @well-noted thank you for this, and apologies for being late to engage thanks to the holidays.

Firstly, this is an interesting and determined attempt to solve a significant problem, and bring considerable benefits to users.

I have some questions and possible reservations about the approach of preloading the entire media file using XMLHttpRequest. (I first saw an earlier version of the code that was much simpler, and haven't been able to fully review the new code and so its possible these points no longer apply).

I think that XHR and the raw video/audio tags have different behaviour with respect to CORS which would present a backwards compatibility issue
My understanding is that seeking through remote media on the web is supposed to be done by the player using HTTP range requests. Doesn't loading the entire file at once break this mechanism?

There are also a few issues with the implementation, most of which I think have already been discussed:

Parsers have no business interacting with the DOM. They should be pure functions that convert a string/type combination into a parse tree
The "th-page-refreshed" hook is a very low level hack that is only provided for 3rd party plugins to use as a last resort, and is not appropriate for use by the core
Note that "th-page-refreshed" only gets called for the main window, and not for secondary windows

If we were to focus on core changes to enable this functionality to be safely shipped as a plugin, a possible approach would be to refactor the video and audio parsers so that they generate special <$video> and <$audio> widgets. The core implementation would simply produce the associated HTML element, but you'd be able to ship a plugin that overrode those widgets with a more sophisticated JS implementation. We could later consider that implementation for the core.

Stepping back, the fundamental problem with offering users a reliable and consistently good experience for video playback is really quite fundamental: TiddlyWiki works on the basis that any part of the DOM tree can be destroyed and recreated at any time which is utterly at odds with the needs of stateful HTML elements such as <video>.

In many cases, we can work around the problem by ensuring that all the required state is stored in the wiki and so if the element is destroyed it can be recreated with the same state. This is accomplished using a custom widget rather than the generic HTML element widget. For example, that is how the edit text widget doesn't lose content when the page is refreshed; a plain <input> element on the other hand will lose its content when the page is refreshed.

However, that technique isn't good enough for media playback. Humans are sensitive to very short glitches, particularly in audio. So at the back of my mind has been the idea of a separate media player JS component that renders the <video>/<audio> element outside of the main render tree (the controls can still be rendered in the usual way). With this approach the <$video> element would be a button that send a widget message to initiate playback of a particular tiddler.

pmario · 2025-01-06T14:05:52Z

However, that technique isn't good enough for media playback. Humans are sensitive to very short glitches, particularly in audio. So at the back of my mind has been the idea of a separate media player JS component that renders the

I was also thinking about "interrupt free" state. My first thought was to render the video/audio elements in the root-widget, so it will be "outside" the main refresh tree. (That's similar to what Jeremy suggested)

The next thought was: Let's open a new window for the video or audio file. -- This may be able to keep the window alive even if the tiddler is closed in the story river. (That's my personal favourite)

Or the other way around: Let root handle the video-tag and create a new small window with the controls only. This would allow us to move the controls around. IMO that's handy for audio-only UI. Where the UI would be completely out of the way.

well-noted · 2025-01-06T21:33:27Z

Parsers have no business interacting with the DOM

Heard, I have actually been discussing with etardiff storing the values as statetiddlers, which I believe would remove most of the need for DOM manipulation.

Let's open a new window for the video or audio file.

Personally, this solution would not work for me unless the time-values could still be accessed from within the wiki (And, still not quite as elegant, to my mind) as having the media element as an overlay on the page that allows for closing the story tiddler, as the audioparser modifications I have made currently allow for. I believe it is core to the philosophy of tiddlywiki that all values should be able to be referenced and transcluded in any context.

a possible approach would be to refactor the video and audio parsers so that they generate special <$video> and <$audio> widgets.

Yes, I believe that, using this as a proof-of concept, this PR could be refactored into a plugin if it were possible to intercept the video rendering through the widget system. and agree that it has the benefits of maintaining backward compatibility, while also allowing for additional features that might be helpful options to end-users but would unnecessarily bloat the core.

I am satisfied with this as a direction and am happy to begin proceeding in refactoring as a plugin, unless there is a more elegant core solution on the horizon?

Jermolene · 2025-01-07T16:02:36Z

Yes, I believe that, using this as a proof-of concept, this PR could be refactored into a plugin if it were possible to intercept the video rendering through the widget system. and agree that it has the benefits of maintaining backward compatibility, while also allowing for additional features that might be helpful options to end-users but would unnecessarily bloat the core.

Hi @well-noted would you be interested in drafting a PR for this? Let me know if you need any more information beyond my comments above.

well-noted · 2025-01-11T02:34:09Z

@Jermolene, lmk if this would be appropriate.

Allow seeking throughout audio files

da3ef6e

well-noted closed this Dec 22, 2024

well-noted reopened this Dec 22, 2024

well-noted marked this pull request as draft December 22, 2024 20:53

Allow time seeking in videoparser

d084944

well-noted changed the title ~~Allow seeking throughout audio files~~ Allow seeking throughout video files Dec 23, 2024

well-noted marked this pull request as ready for review December 23, 2024 00:59

Allow special characters in video titles to buffer and timeseek witho…

49d6394

…ut error

pmario reviewed Dec 23, 2024

View reviewed changes

Improve loading, store/retrieve timestamps field

a53fe9d

pmario reviewed Jan 5, 2025

View reviewed changes

well-noted closed this Jan 6, 2025

well-noted mentioned this pull request Jan 11, 2025

Allow intercept of videoparser by widget and pass attributes #8892

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow seeking throughout video files #8844

Allow seeking throughout video files #8844

well-noted commented Dec 22, 2024 •

edited

Loading

github-actions bot commented Dec 22, 2024

netlify bot commented Dec 22, 2024 •

edited

Loading

github-actions bot commented Dec 22, 2024

arunnbabu81 commented Dec 23, 2024 •

edited

Loading

pmario left a comment

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

pmario Dec 23, 2024

buggyj commented Jan 4, 2025

well-noted commented Jan 4, 2025

well-noted commented Jan 4, 2025 •

edited

Loading

well-noted commented Jan 5, 2025 •

edited

Loading

well-noted commented Jan 5, 2025

pmario Jan 5, 2025

Jermolene commented Jan 6, 2025

pmario commented Jan 6, 2025

well-noted commented Jan 6, 2025 •

edited

Loading

Jermolene commented Jan 7, 2025

well-noted commented Jan 11, 2025


		xhr.send();

		video.addEventListener('loadedmetadata', async () => {

Allow seeking throughout video files #8844

Allow seeking throughout video files #8844

Conversation

well-noted commented Dec 22, 2024 • edited Loading

github-actions bot commented Dec 22, 2024

netlify bot commented Dec 22, 2024 • edited Loading

✅ Deploy Preview for tiddlywiki-previews ready!

github-actions bot commented Dec 22, 2024

arunnbabu81 commented Dec 23, 2024 • edited Loading

pmario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buggyj commented Jan 4, 2025

well-noted commented Jan 4, 2025

well-noted commented Jan 4, 2025 • edited Loading

well-noted commented Jan 5, 2025 • edited Loading

well-noted commented Jan 5, 2025

Choose a reason for hiding this comment

Jermolene commented Jan 6, 2025

pmario commented Jan 6, 2025

well-noted commented Jan 6, 2025 • edited Loading

Jermolene commented Jan 7, 2025

well-noted commented Jan 11, 2025

well-noted commented Dec 22, 2024 •

edited

Loading

netlify bot commented Dec 22, 2024 •

edited

Loading

arunnbabu81 commented Dec 23, 2024 •

edited

Loading

well-noted commented Jan 4, 2025 •

edited

Loading

well-noted commented Jan 5, 2025 •

edited

Loading

well-noted commented Jan 6, 2025 •

edited

Loading