SplitmediaLabsLimited / xjs

XSplit JS Framework. Make plugins for XSplit Broadcaster, quickly and easily.
Other
39 stars 11 forks source link

Bugs? setValue on Source for video/audio #295

Closed BrendaH closed 4 years ago

BrendaH commented 4 years ago

I'm trying to write an extension to easily replace parts of the paths of my sources. Example: c:\content\a=b\q=z\media.mov can be changed into c:\content\a=c\q=x\media.mov after the user chooses the settings for a and q. And this for a lot of sources (so no inclination to do this by hand..)

For this I use on Source the getValue, then a regex for the replacement and then setValue for the changed path. However, I noticed a couple of oddities:

1 - If your source has an extension in capitals (.MOV instead of .mov) it breaks, because on line 10261 of xjs.js a VIDEO_REGEX is used case-sensitive. Since adding a source with capitalized extension works perfectly fine in Xsplit I think it a bit weird that you can not set a value with such extension?

2 - For looping videos for some reason (unclear to me) the file path is followed by stuff in the form of *0*0. I'm talking about the item= value in the generated xml file that is the .BPres. Trying to replace this item= value (which is what setValue seems to do) does not work because again the VIDEO_REGEX fails (not expecting anything after .mov). However: if Xsplit puts this stuff behind the file path for some reason, why can't xjs.js deal with it? Just leave it be?

3 - I noticed that setValue clears the cue points and sets the name as well, next to editing the value. https://github.com/xjsframework/xjs/blob/a1197da3015af103462a8d7ab4dff60c6b07e329/src/core/source/iplayback.ts#L653 Why is this? There's separate methods to do those things. If I want to change the path of a looping video, but not the cue points this is made impossible this way. (Of course I could fix this for my case by (re)setting the cue points after changing the value, but setValue obviously does more than setting just the value, so the expectation of what it does is broken)

BrendaH commented 4 years ago

I had to dive a bit deeper into the code and found out that there is yet another list where extensions for video/audio are stored: https://github.com/xjsframework/xjs/blob/a1197da3015af103462a8d7ab4dff60c6b07e329/src/core/source/media.ts#L11 This list is also used in a regular expression: https://github.com/xjsframework/xjs/blob/a1197da3015af103462a8d7ab4dff60c6b07e329/src/util/sourcetyperesolve.ts#L37

So if at some point in time a new extension needs to be added, it needs to be added in two places.

I did have trouble getting the right type of source (MediaSource) back for my videos and now I get why: this other regex is also case sensitive and thus my video .MOV files fell through and ended up as a regular Source object. This particular place in the code does not have the *0*0 issue as described in point 2 in my post above (since there is no 'till the end of string' marker), however the dot (.) is not escaped before using it in a regex, so your.move.now.png could possibly be matched to be a video file, or even bla_imove_something (since a dot is 'any character')?

I have not searched for it, but I can imagine that this whole structure of finding out what kind of Source or Item something is lives in two places (one for Source and one for Item) and thus could/should both be fixed?

SML-MeSo commented 4 years ago

Hi! Thanks for the inquiries and reports! :) To address them:

1 - definitely a bug, for fixing

2 & 3 (looping index and extra stuff for setValue) - This merits a discussion as these behaviors are somewhat as designed. The XJS framework, especially when it comes to preventing unwanted cases, emulates the source properties dialog (the dialog shown when right-clicking a videoitem). For the *0*0 stuff, this handles playlist index and current loop count for the video which we always reset in the source properties dialog. It was deemed at the time of its development for the source properties dialog that resetting it to 0 is more practical than to allow setting of the current looping index. As to why a media source has this *0*0 and additional details regarding this, please check comments for #296. For the extra stuff in setValue, this is also based on the source properties dialog behavior when changing the media source. The changing of prop:name, I think, is quite understandable enough. As for the cuepoints part, I think it was a conscious decision to remove them on source change, due to the following reasons:

4 (regex syntax error) - most likely a bug (haven't checked myself), for fixing. As to why the resolution "lives" in separate code blocks, this is honestly a workaround on our end. We tried to unify them in a single utility, but we encountered some bundling problems due to how our mixins work. Putting them together causes the Source class to reference a still yet-to-be-defined Item class leading us to the parallel codes approach until we handle that properly (which we want to, just that it needs more additional time than we can allot)

matthijskooijman commented 4 years ago
  1. Wrt the *0*0 stuff for looping videos, I think this is essentially what #296 is about, and the solution proposed there (make MediaSource smarter by chopping off the *0*0 in getValue and re-adding it (and updating the single-item-playlist) in setValue) would be fine for our case. This would probably reset the *0*0 suffix, but the playlist index is always 0 and an implementation detail, the loop count is not relevant for us, so fine to reset that to 0 (if anyone needs to preserve that across setValue() calls, a separate get/setLoopCounter or something could be added).

  2. Wrt name & cuepoints: Changing / resetting those is probably fine, but might need to be documented. For our case, if we know we need to preserve cuepoints or name, we can just save and restore it manually.

  3. Having the regex duplicated makes things harder to maintain, but is not otherwise a bug. I can imagine that circular dependencies make this hard to fix. What I think does need to be fixed is the case sensitivity, dot and the anchor-at-the-end behaviour (but when the regex is anchored at the end, the *0*0 prefix must be stripped or matched as well).

SML-MeSo commented 4 years ago

Action Plan: