godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Implement `get_stream_length()` for built-in supported video formats #10148

Open xana43 opened 2 months ago

xana43 commented 2 months ago

Describe the project you are working on

I'm currently working on a 3D psychological horror game that makes use of video extensively as a method for story telling.

Describe the problem or limitation you are having in your project

I want some of these videos to be fully interactive, as of right now I can only start and stop the videos, I can't seek videos, and I can't do verification on it's audio (I have a system set up to where if I want to I can make the video's audio play through an audio stream player, but as of right now any audio can be used with any video and not only the audio that's the exact length as the video can be used). As per the documentation the get_stream_length() and set_stream_position() isnt implemented at all for theora

Describe the feature / enhancement and how it helps to overcome the problem or limitation

implementing the stream length and allowing seeking can save a lot of confusion because to me seeing as getting the length, getting the current positon etc. is already implemented for the audio streams, why wouldn't it be implemented for the video streams. I get that in the documentation it says it's not implemented but that was after a lot of confusion as to why A. get_stream_length() just returns 0 and B. why seeking doesn't do anything. especially since at a cursory glance Theora and Vorbis seem to be very similar.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I'm not really well versed in decoder programming, but couldn't a similar solution used for Vorbis work for Theora? or if that couldn't work, couldn't using the video's audio track work for at least determining the length of the video and where the current video stream is?

If this enhancement will not be used often, can it be worked around with a few lines of script?

As per documentation this can be worked around using a separate GDExtension that implements it's own format

Is there a reason why this should be core and not an add-on in the asset library?

most people who either are starting out with godot, or have been working on a project for a while will most likely be using whatever format is by default built in, and will continue using it until they eventually reach some point where they do need to get the length of the video or seek the video, in this case there's only a few options, either get a GDExtension that implements these functions and then re-encode all of their videos, completely pivot their idea to no longer need that functionality, or move completely away from godot as functionality like this is present in other engines.

for the most part beginners in this situation will be inclined to move to a new engine

AThousandShips commented 2 months ago

I'd say this isn't unimplemented just because no one has bothered to do so, so I think some suggestions about how this would be done would be necessary here, or some indication that it is possible and that the lack of it is an oversight and not due to limitations

Edit: Looking at the theora library I can't find any clear way to get the length info

xana43 commented 2 months ago

maybe a suggestion could be to do what is done for the Vorbis audio stream? according to the documentation yes there's no real way to get the length info (which confuses me tbh) but it does seem to have structures for getting ogg packets. I did find this https://github.com/KTXSoftware/theora/blob/master/examples/player_example.c#L235 not sure if it helps or not

AThousandShips commented 2 months ago

The format is likely for streaming generally, and is built for efficiency in getting data right now and not caring about the whole content, I think the way it gets the ogg packets is also gradual

I suspect that getting the full length requires processing the whole video and then saving the length, which would be wasteful

AThousandShips commented 2 months ago

Note also that there's talk about moving the whole system out of core, which means we might not want to make changes like this for something that might be dropped

xana43 commented 2 months ago

I guess we'll just have to see what happens. Though I belive there's got to be a way. With enough time and determination i'm pretty sure someone can figure it out.

AThousandShips commented 2 months ago

The trivial but hacky solution is just to process through the whole file and storing the time, but there doesn't seem to be any simple way to do it, so it'd require some workaround or hacky solution unless someone finds a good solution, that seems to be the reason this wasn't implemented in the first place. If there was a straight forward way it'd had been done IMO

But I think the work would have to depend on what we decide we want to do with the class itself, it'd be better to leave it to an extension if we decide to split it off and not take up development time

Tl;Dr; it's less of a question of "can it be done", but rather "can it be done well"

pgorley commented 3 days ago

I'm going to chip in here because I just lost an hour of my life to this. Why do we expose a method that has no underlying implementation? I suggest get_stream_length() be removed because it will always return zero. I'd rather not have other people bump into this the same way that I did.

Why offer up a method, especially in the autocomplete, if it's not going to do what it says it's going to do? Yes, I understand the documentation is there, but I would have been spared all that time tracking down the issue had I not been suggested to use a method that adds no value.