ofVideoPlayer using fs::path

[dimitre]

this PR intends to use fs::path internally for video player functions (load, loadAsync, loadMovie [[deprecated]] )

[roymacdonald]

For the parts where you just add override it is ok. but where you change from string to fs::path I would rather deprecate the function with string and make a new one with fs::path so this does not break other classes that might inherit from any of these.

[ofTheo]

@dimitre looks good to me!

[roymacdonald]

I insist that this should not be done this way. and this merge needs to be reverted. It is a virtual function to which you are changing its signature. You are changing the interface and it will break other's code. It WILL break addons and other code that are working perfectly well with a string. For example here.

Saying that you can offer updating the addon is not the correct solution, since that is one of the many derived classes where it could happen. OF has a lot of important issues that need to be fixed right now, and introducing this big source of problems with literally zero benefits is really unnecessary.

I say zero benefits because no where in the updated code is making use of the useful functionalities that of::filesystem::path has.

I am not sure if it is possible to have two functions named the same, like bool load(string fileName) and bool load(const of::filesystem::path & fileName) and have the compiler to figure out which one to call in case a string literal is passed to it. Which make is a bit harder to just add another function.

@dimitre @ofTheo I have to insist, we need to bring this back.

[roymacdonald]

Guys, I have added this https://github.com/openframeworks/openFrameworks/pull/8141 PR to revert this change. I think this is serious, as well as all the changes done where fs::paths has been placed instead of an std::string. I see that most of these are being done by @dimitre (don't get me wrong, I really appreciate the energy and time you are putting into OF's development, but we can not do this breaking changes without a clear intention and ways of mitigating it). I have already spent several days working on fixing PG and I can tell that most of the problems arise from dropping fs::path into it as if it was interchangable class std::string. Please remember that it is not! you can create an fs::path from an std::string, and automatically convert, but you can not do the other way around and that is what breaks it.

@dimitre @ofTheo @ofZach @bakercp @artificiel @2bbb @danoli3 (I'd rather not ping you directly but I think this is serious )

BTW, I do have the privileges needed to just go and merge my PR, but I'd rather be respectful and find out your point of view before doing such

[ofTheo]

@roymacdonald I think it would be helpful to describe what about this PR is a breaking change?

A list of issues or regressions caused by this PR would be really helpful. Otherwise it's hard to know if this is something that can be addressed or is just simply a large breaking change.

ps: One of the reasons we're trying to move towards fs::path is so that OF can better support a wider range of languages / locales. If you have suggestions to making the switch in a cleaner way would be great to hear too!

Thanks so much!

[dimitre]

Yes this is a breaking change. we already merged a lot of internal functions now returning fs::path, like ofToDataPath for example. with this one and this other PR https://github.com/openframeworks/openFrameworks/pull/7435 we can finish streamlining fs::path as the default data type for paths in OF Core and have 0.13 release out (breaking release in semantic versioning) I can help porting any addon with compatibility with OF 0.12- and 0.13

[dimitre]

I'm good if it is decided differently but I think it is important because all changes to make it work internationally are already ok

• make using relative paths, so spaces in path in user name can be allowed, like /Users/Nicanor Parra/ofw is allowed.
• fs::path used in files, so if Nikakoi from Georgia decides to load a video using windows called ნატალია ბერიძე.mov it is OK

[ofTheo]

Thanks @dimitre !

• In the future I think it would be good for all PRs to list any breaking changes. So its clear when its merged that there is an intentional break ( for example I was unaware this was a breaking change PR )
• Also, as I am still in the dark :), what is the breaking change here? What could you do that you now can't do?

Thanks!

[dimitre]

not breaking in the sense of returning paths, it will work the same as before. only to derived classes like ofxHapPlayer. if it is everything ok with this PR here I submit a PR there. we did before in ofxSyphon project with ARC changes and they applied to master once 0.12 was released

[roymacdonald]

I am aware of how helpful it can be to have better locale support, but if that breaks previous code is not good. @dimitre thanks for offering your help to update addons but that is not the proper way to go, there is way much more than just addons that will break.

So, in this case, any class derived from ofVideoPlayer will break. Let alone fixing ofxHapPlayer, it is not about the specifics but rather the design choices. If you do it for this then you will do for anything else. Changing the interface signature is a delicate matter.

So, in the case of just throwing in fs::path as a direct replacement for std::string as the argument type in any virtual function is a bad idea, since the virtual function is meant to be overriden, thus it will break any class that does so. If you encounter this problem, thinking that is is a small change that you can do in fix it is not a good way of thinking, because you are spending your time, but this also can replicate to a much larger amount of people.

Then, also changing the return types of functions from std::string to fs::path will break stuff. As mentioned, you've changed ofToDataPath to return type, so now anywhere that you have, somethings like std::string myPath = ofToDataPath("/some/path/file.ext"); it will stop working. And fixing that is a pain in the ass,TBH. Code that was perfectly working now does not at no added benefit. And tracking this is a lot harder than spotting the virtual function issue as you probably need to compile in order to get the error (you can not just run a script that makes this update for you)

The same happens for public and protected class members.

Being able to run some old code without doing any change. in your current OF setup is really nice. But now , one of the really interesting features of OF is the amount of addons available. A lot have not been updated for quite a while and still work fine, but because of these changes these will not.

Also, a lot of the issues that I've been fixing in PG (which is quite broken ATM) come from just throwing in std::filesystem::path as a direct replacement for std::string, which is not.

I see how useful it is to have std::filesystem::path but because of its introduction everything breaks is a bad idea.

So, I suggest that we revert all the changes that introduced std::filesystem::path in the master branch, and we keep those in a development branch. Then, once we are happy with all these we can merge, but we do it all at once and not as random commits that are all over the place. As well we must provide ways for mitigating any breaking change, from proper documentation (not just a comment on the source code but more like what arturo did when we switched from ofVecf to glm::vec, like this) to scripts that can automate parts of this update, and anoucing it properly.

[ofTheo]

Thanks Roy - I think I wasn't aware of all the issues with inherited classes ( especially when we are making changes in classes that are designed to be inherited ).

For the ofVideoPlayer use case ( as an example ) is there a way to keep a legacy virtual function that can still allow un-updated addons to work with a deprecated warning?

ie:

    virtual bool load(const of::filesystem::path & fileName){} //drop pure virtual in favor of allowing one or the other to be supported

    OF_DEPRECATED_MSG("Please use filesystem arg version!",   virtual bool load(const string & fileName){}  );

Or will these be too similar and result in errors due to ambiguity?

[ofTheo]

Diving more into this and with some help from chatGPT - it looks like we could use a proxy class to essentially support both string returns and of::filesystem::path returns across all our classes.

It's definitely a bit more complicated but it could in theory support people doing:

std::string path = ofToDataPath("blah.txt"); 
of::filesystem::path path = ofToDataPath("blah.txt"); 

even maybe: std::wstring path = ofToDataPath("blah.txt");

#include <filesystem>
#include <string>

// Proxy class to handle implicit conversion to std::string and std::filesystem::path
class FilePathProxy {
    std::filesystem::path path_;
public:
    FilePathProxy(const std::filesystem::path& path) : path_(path) {}

    // Implicit conversion to std::string
    operator std::string() const {
        return path_.string();
    }

    // Implicit conversion to std::filesystem::path
    operator std::filesystem::path() const {
        return path_;
    }
};

// Function that returns the proxy object
FilePathProxy getFilePath() {
    return FilePathProxy("/some/path/to/file.txt");
}

int main() {
    // Existing code using std::string continues to work
    std::string filepath = getFilePath();  // Converts to std::string automatically

    // New code can use std::filesystem::path
    std::filesystem::path path = getFilePath();  // Converts to std::filesystem::path automatically
}

Curious what you think @dimitre @roymacdonald @artificiel ?

Ideally we could modernize while maintaining backwards compatibility.

[artificiel]

std::filesystem::path already has an implicit conversion operator to std::string, so

std::string myPath = ofToDataPath("/some/path/file.ext");

works, as it is equivalent to:

std::string myPath = ofToDataPath("/some/path/file.ext").string();

changing the signature of methods is still a valid problem, as pointed out, specifically for virtual ones. perhaps there is a transitory approach. @roymacdonald near the top in this issue suggested not to change the signatures but to deprecate the std::string signatures and add a distinct ::path one. the problem with that is that end-user writing

movie.load("my.mov"); // will generate a deprecation warning

as "my.mov" is a char *, which gets converted to std::string, which would be deprecated.

so maybe adopting a 3 signatures pattern and implement things like:

[[deprecated("favour of::filesystem::path for path representation")]]
bool loadMovie(std::string filename) {
  return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(const char * filename) {
  // bypasses char->string conversion for end-user convenience in C++ source text
  return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(of::filesystem::path() & path) {
  // actual implementation;
}

someone writing

std::string mov {"my.mov"};
movie.load(mov); // will show the deprecation, which will make sense as the type is explicitly stated
//
movie.load("my.mov"); // no deprecation and the char * is converted to fs::path

so ofxHapVideoPlayer works, nothing has to change in what is already implemented out there, and ::path awareness is raised through deprecation.

[artificiel]

I'd also like to take the occasion to point out that a larger-scale issue is raised here by @roymacdonald relating to management of the master branch (how to organize intent, momentum, testing, information dissemination and the high-level decision process of balancing moving ahead and keeping things the same).

it needs another context for discussion, and it is not clear that the GitHub Issues (or Discussions) is the ideal one as there are many considerations that are easy to have out of focus, then at some point brought back, leading to dead-ends as unravelling the "significant meat" of prior exchanges becomes more complicated than taking the actual decisions. I'm not suggesting anything just agreeing that the management of /master is not ideal and needs a refresh. I'm also of the opinion that things must evolve as the C++ core is more and more smelling of an old timer's dorm, which is not conducive to rally new users.

[ofTheo]

@artificiel thanks for this input! That's super helpful. I actually did not already know that of::filesystem::path had an automatic string conversion.

so maybe adopting a 3 signatures pattern and implement things like:

[[deprecated("favour of::filesystem::path for path representation")]]
bool loadMovie(std::string filename) {
return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(const char * filename) {
// bypasses char->string conversion for end-user convenience in C++ source text
return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(of::filesystem::path() & path) {
// actual implementation;
}

@roymacdonald would something like the above address the inherited class concerns with the PR?

[roymacdonald]

Thanks @ofTheo and @artificiel for your replies. Well, it happens that I was wrong aboutstd::string myPath = ofToDataPath("/some/path/file.ext");, that one indeed works.

Although, this

[[deprecated("favour of::filesystem::path for path representation")]]
bool loadMovie(std::string filename) {
  return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(const char * filename) {
  // bypasses char->string conversion for end-user convenience in C++ source text
  return loadMovie(of::filesystem::path(filename));
}
bool loadMovie(of::filesystem::path() & path) {
  // actual implementation;
}

does not. It ends in an infinite loop if I pass a cstring literal. I am not sure what would be the correct way of handling this. I need to do more research about it. But I would guess that we are not the first ones to face such.

I kinda like the idea of a proxy class. I will take a look at it.

Ideally, having classes like ofVideoPlayer, where load function is virtual , add a deprecation warning if the string version of it is being used, but still alllow it to work. Probably using some sort of template if_enabled condition could help. As said It needs to be addressed properly. But because of such I think we need to revert this commit, and others until we have a solid fix.

I also found that following breaks now:

void f(std::string str){
    cout << "f : " << str << "\n";
}

void g(std::string& str){
    cout << "g : " << str << "\n";
}

void h(const std::string& str){
    cout << "h : " << str << "\n";
}

    auto path = ofToDataPath("something.png");
    f(path);
    g(path); // this does not compile.
    h(path);

Which makes sense.

[roymacdonald]

As for @artificiel mentions about the use of branches, I agree, that goes elsewhere, nevertheless it needs to be addressed.

[artificiel]

@roymacdonald it should not require templates; is it possible that your test methods were not fully qualified? (the code above was written in-github-issue so untested etc and also I used the already-deprecated loadMovie()) but:

[[deprecated("instead of std::string favour of::filesystem::path for path representation")]]
bool ofVideoPlayer::load(std::string filename) {
  return loadMovie(of::filesystem::path(filename));
}
bool ofVideoPlayer::load(const char * filename) {
  // bypasses char->string conversion for end-user convenience in C++ source text
  return loadMovie(of::filesystem::path(filename));
}
bool ofVideoPlayer::load(of::filesystem::path() & path) {
  // actual implementation;
}

works here.

with some logging it shows:

[notice ] using const char * filename: a.mov
[notice ] using const of::filesystem::path & fileName: "a.mov"
video file loaded at 360 x 360 @ 50.000000 fps
[notice ] using std::string filename: b.mov
[notice ] using const of::filesystem::path & fileName: "b.mov"
video file loaded at 360 x 360 @ 50.000000 fps
[notice ] using const of::filesystem::path & fileName: "c.mov"
video file loaded at 360 x 360 @ 50.000000 fps

[roymacdonald]

I tested a general case function, having the same name. But different arguments, since we might have to apply the same to so much more classes. It ends in an infinite loop. My test was vague but regardless of it it failed and that proves that it needs to be handled carefully.

[artificiel]

I'm sorry if I'm not understanding what's being proven here; can you share your test code?

[artificiel]

also @dimitre: could you perform a quick audit of the virtual functions in OF core that are affected by the change from std::string to of::filesystem::path? it does not have to be 100% accurate; just so we have a quantitative data point.

[dimitre]

@artificiel not any other virtual function I remember.

[artificiel]

a bit of grep action revealed 3 files with methods with such virtual interfaces: ofVideoBaseTypes, ofURLFileLoader, ofSoundBaseTypes. (of course that does not rule out inheriting something not specifically declared virtual which would create an overload instead of an override; but it does not cease to work — and if override is used, it will warn. still maybe there are core classes often subclassed with overrides for FS access methods that would gain from a similar deprecation warning?

[ofTheo]

@artificiel @dimitre

After thinking about it a bit I think the video player should take both string and filesystem args without deprecating one over the other.

A lot of users will be doing movie.load("myVideo.mp4" ); or even storing that in a string and passing in and that shouldn't be discouraged. I think pushing them to use of::filesystem::path is not really needed.

But having filesystem::path for say dragging a file into a window is great - because there will be less issues with international filesystems.

So maybe we just need the two functions without the deprecation?

[artificiel]

I'm 50/50, as there are other advantages to be aware of ::path notably all the builtin stuff you can do with it. but I won't argue with a no-deprecation decision.

to be sure though, ofVideoPlayer itself has no virtual methods, those are defined in ofBaseVideoPlayer, which, incidentally (and somewhat oddly), is not inherited by ofVideoPlayer...?

adding virtual public ofBaseVideoPlayer in the ofVideoPlayer declaration reveals that the class conforms to the interface (all required methods are implemented) but that means it's currently not polymorphic (overridden methods won't be called from a base class pointer). so I guess ofVideoPlayer should inherit ofBaseVideoPlayer?? of course it's pretty specialized but if we're toying with virtual I guess it should be made correctly.

and then should these overridden methods be re-virtualized at the ofVideoPlayer level? (that's what ofxHapPlayer does). (also note that HPVPlayer does not bother inheriting ofBaseVideoPlayer; it almost conforms but changed some return types).

BUT it raises another point: if the new ::path methods are pure virtual (as currently) then it break ofxHapPlayer because it will be unimplemented. So I presume it means the new methods should not be pure, just virtual?

here is what happens when inheriting ofBaseVideoPlayer:

[artificiel]

currently these are the methods to be dealt with:

./utils/ofURLFileLoader.h:  virtual int saveAsync(const std::string & url, const of::filesystem::path & path) = 0;
./video/ofVideoBaseTypes.h: virtual bool load(const of::filesystem::path & fileName) = 0;
./video/ofVideoBaseTypes.h: virtual void loadAsync(const of::filesystem::path & fileName);
./utils/ofURLFileLoader.h:  virtual ofHttpResponse saveTo(const std::string & url, const of::filesystem::path & path) = 0;
./utils/ofURLFileLoader.h:  virtual int saveAsync(const std::string & url, const of::filesystem::path & path) = 0;
./sound/ofSoundBaseTypes.h: virtual bool load(const of::filesystem::path& fileName, bool stream = false)=0;

[dimitre]

I now can see the ambiguities mentioned by @roymacdonald this PR works ok https://github.com/openframeworks/openFrameworks/pull/8142

[dimitre]

great @artificiel. I think at least the functions in ofURLFileLoader.h doesn't need to be virtual. At least a search in github with the code "public ofURLFileLoader" returns nothing

[artificiel]

and also perhaps just for the sake of the deprecation-or-not discussion: while paths are easily represented by a string (and yes "it currently works"), a filesystem path is not just a string — it is an object with a specific intent. this topic is important to be discussed within the learning process (all examples should be upgraded to path). it's not just about path vs string, but reasoning about objects and how the type of an object informs about it's function.

also it's one thing to type a bunch of std::strings in source to load static assets, but any code relying on "getting a path" should be returning a ::path (including ofToDataPath). if std::string is propped around as an OK equivalent container for paths, risks are high that they will get converted implicitly and users will be making path->string->path travels without noticing.

[dimitre]

the inheritance of ofVideoPlayer can be greatly simplified. I studied this classes in the past. i think is it is complex by historical reasons, my supposition is in the past ofVideoPlayer was the video player and now it is ofVideoPlayer->player the one who plays, to allow different backends so this classes are kinda similar but they doesn't serve the same purpose which is confusing.

[ofTheo]

So I presume it means the new methods should not be pure, just virtual?

Yes definitely! That was my thinking too @artificiel

[artificiel]

@dimitre hm that's strange too: ofURLFileLoader is not inheriting ofBaseURLFileLoader — but beyond polymorphism these kinds of abstract classes provide no actual functions but enforce an interface. Like videoPlayer, if OF defines an interface (contract) to be a videoPlayer and lists N methods to implement in order to be conformant, then ofVideoPlayer should be the first to do so...?

[roymacdonald]

@artificiel thanks for digging into this. It is really interesting that ofVideoPlayer does not inherit from the ofBaseVideoPlayer. Also the ofURLFileLoader issue seems weird. I just wonder if these were like this by design or not.

[artificiel]

@roymacdonald yes there is a design question also maybe in the naming -- "base" suggests functionality-to-be-expanded while "abstract" suggests interface enforcement, with required methods marked pure. Seems these "base"-named classes are more of an abstract nature (that's obviously not black vs white but in the spirit of self-documenting design, it helps to have clear names). And it seems in the case of ofVideoPlayer that the interface was respected, but without inheriting anything. Beyond not making things explicit with override, it also breaks polymorphism (if that is to be expected between of(Base)VideoPlayer subclasses, maybe not... again it's more of a decision than requirement).

[roymacdonald]

@artificiel Sure. It is just strange. Although I dont clearly see why having ofVideoPLayer to subclass ofBaseVideoPlayer will break polymorphism. Thinking about it, ofVideoPlayer does not need to inherit from any base class, because it is not being inherited from, at least in the OF core, while it is it's internal player object that is subclassing ofBaseVideoPlayer. Having ofVideoPlayer to inherit from another class might have some sort of implications or performance issues or what ever. I think it is better to leave as is, unless there is a really good reason to make ofVideoPLayer to inherit from ofBaseVideoPlayer.

[dimitre]

I too agree ofVideoPlayer should inherit less classes. maybe ofBaseDraws and not much more. it is a video player container. simplifying we can have simpler objects with no detriment to anything. there is a very complex chain of inheritance without much functionality

[artificiel]

yes ofBaseVideoPlayer has no implementation whatsoever — but inheriting an abstract class is not about bundling functionality but:

(a) signifying the adhesion to an interface, and fulfill some requirements (pure virtual)

(b) allowing the dynamic typing of overridden methods. So for instance (pseudo-code)

std::vector<std::shared_ptr<ofVideoBasePlayer>> players;
players.push_back(make_shared<ofVideoPlayer>());
players.push_back(make_shared<ofHapPlayer>());

for (const auto & p: players) p->draw(); // the overloaded draw will be called, even if p is of `ofVideoBasePlayer` class

in this case (a) is more important. that being said, maybe it's not meant to be inherited but it's really a mystery as to why the class is obviously written as compliant, but not inherited.

[roymacdonald]

I think that it works fine. It´s been like this for quite a while and doing any changes might lead to trouble. I think that we should leave it as is and focus on other issues that are way much more important. :)