Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugs? setValue on Source for video/audio #295

Closed
BrendaH opened this issue Apr 15, 2020 · 4 comments
Closed

Bugs? setValue on Source for video/audio #295

BrendaH opened this issue Apr 15, 2020 · 4 comments

Comments

@BrendaH
Copy link

BrendaH commented Apr 15, 2020

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
Copy link
Author

BrendaH commented Apr 15, 2020

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
Copy link
Collaborator

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:
- Cuepoints marks significant parts of a video, e.g. you pause or cut at a certain part of the video and resume at the next. At least in our discussions while developing for it, we deem that it is deeply related to the actual video, thus when changing the video itself, you lose that connection, i.e., a significant part (at a certain time) of one video may not be relevant for the new one.
- Different videos may have different durations, such that the cuepoints for one video may exceed the whole duration of the new one. Coupled with the above assumption, we deemed resetting to be more practical than invalidating each cuepoint based on the new duration.

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)

@SML-MeSo SML-MeSo self-assigned this Apr 20, 2020
@matthijskooijman
Copy link

matthijskooijman commented Apr 20, 2020

  1. Wrt the *0*0 stuff for looping videos, I think this is essentially what Bug? Looping video is Playlist but not recognized as such #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
Copy link
Collaborator

SML-MeSo commented May 5, 2020

Action Plan:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants