lay295 / TwitchDownloader

Twitch VOD/Clip Downloader - Chat Download/Render/Replay
MIT License
2.68k stars 261 forks source link

VOD downloads "Crop Video" function is broken #935

Closed Nutri626 closed 9 months ago

Nutri626 commented 9 months ago

Checklist

Edition

Select an Option

Describe your issue here

since latest update of the program vod download is bugged, for example you cant normally download 3 second clip if you want to, for 7-8 second clip it only downloads 1 second, another bug is that start and end time function is not working well, it randomly cuts last 4-5 second and adds first 2-3 second when in the past it downloaded exact part it that you wanted, clearly something is broken about crop function

Add any related files or extra information here

No response

vaindil commented 9 months ago

I'm looking at this right now. The problem is with the calculation of startOffsetSeconds on these lines: https://github.com/lay295/TwitchDownloader/blob/e48a04b2dd07c089cec63380c9814af17a4f36f4/TwitchDownloaderCore/VideoDownloader.cs#L87-L91

For debugging, I'm using VOD ID 2023900250 with times from 2:00:03 to 2:00:33.

Lines 87-89 get the initial offset location by calculating the total duration of all parts of the stream leading up to the one that was requested. In my example, this is 7200. Line 91 then subtracts CropBeginningTime from that, which is 7203 for me, so the final value is -3, and that's being passed into ffmpeg later on.

I think the fix should be changing line 91 to this, as CropBeginningTime will always be later than startOffsetSeconds. I'll open a PR shortly.

startOffsetSeconds = downloadOptions.CropBeginningTime - startOffsetSeconds;
ScrubN commented 9 months ago

I think the fix should be changing line 91 to this, as CropBeginningTime will always be later than startOffsetSeconds. I'll open a PR shortly.

startOffsetSeconds = downloadOptions.CropBeginningTime - startOffsetSeconds;

Whoops. If this line change fixes it, then I will merge.

ScrubN commented 9 months ago

No, it should be

startOffsetSeconds += (downloadOptions.CropBeginningTime - startOffsetSeconds)
ScrubN commented 9 months ago

No, it should be

startOffsetSeconds += (downloadOptions.CropBeginningTime - startOffsetSeconds)

I think the code is overcomplicated and we can just use downloadOptions.CropBeginningTime instead of adding segments?

ScrubN commented 9 months ago

I think the code is overcomplicated and we can just use downloadOptions.CropBeginningTime instead of adding segments?

This is wrong.

startOffsetSeconds = downloadOptions.CropBeginningTime - startOffsetSeconds;

This is right.

I didn't get enough sleep last night :/

vaindil commented 9 months ago

No, it should be

startOffsetSeconds += (downloadOptions.CropBeginningTime - startOffsetSeconds)

I think the code is overcomplicated and we can just use downloadOptions.CropBeginningTime instead of adding segments?

downloadOptions.CropBeginningTime is the number of seconds from the beginning of the VOD to start at; in my example it's 7203. That can't be passed directly into ffmpeg, because ffmpeg needs the time from the start of our specific clip. The only way to get that is by adding up all of the chunks before the current one to know how the offset 7203 relates to the start of the first chunk that was actually downloaded.

Similar info is already calculated by GetStreamListCrop, so in the future this could possibly be refactored a bit, but for a quick fix I think we need what I put.