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

REMOVE IT #1452

Conversation

tormyvancool
Copy link
Contributor

2.4 2024-29-10 # Adjusted header style for production
1.0 2024-26-10
# First Release
1.1 2024-26-10
+ Processes Notifications
- /Video/
+ /Videos/
1.2 2024-26-10
- --merge-output-format mp4
+ -S vcodec:h264,res,acodec:aac
1.3 2024-26-10
- 10
+ 2
1.4 2024-26-10
- 2
+ 5
1.5 2024-26-10
- 5
+ 1
# Unified Update
1.6 2024-26-10 - 1
+ 2
+ Version
1.7 2024-27-10
- 'start "" "' from all O.S.s
+ 'start "UPDATE & DOWNLOAD" "' Win
1.8 2024-27-10
- Start = ''
- 1
+ Start = '"'
+ 2
1.9 2024-27-10
+ Check saved project
- 1
+ 2
2.0 2024-27-10
- "chmod +x " .. MainPath
+ 'chmod +x "' .. MainPath .. '"'
# Ordered Variables
- 2
+ 1
+ Apple Trial
2.3 2024-27-10
# Linux execution correction
+ Credits
# 2.1 and 2.2 just trials due issues with Linux and Apple
2.31 2024-28-10
# Binaries directly form the source
2.32 2024-28-10
- yt-dlp
+ yt-dlp_linux
2.4 2024-29-10
# Adjusted header style for production

tormyvancool and others added 12 commits April 3, 2024 16:25
…ormyVanCool_AudioBooks_ChapterMarker.lua

Updated version of the script I started to make 3 year s ago.
It Updates the Chapter Markers create with my script TormyVanCool_AudioBooks_ChapterMarker
…ancool_Audiobooks chapter marker.lua

Renamed the file and removed the 2 empty lines at the beginning of the script
…ol_Audiobooks chapter marker updater.lua

Renamed the file as illustrated convention.
Not any empty row detected here
It prints on an HTML and CSV files located into the REAPER's default, all the installed plugins
It extract a list of all installed VSTs etc, and put the list into an HTML and CSV file into the REAPER's main dir.
2.4 2024-29-10 # Adjusted header style for production
1.0 2024-26-10
    # First Release
1.1 2024-26-10
    + Processes Notifications
    - /Video/
    + /Videos/
1.2 2024-26-10
    - --merge-output-format mp4
    + -S vcodec:h264,res,acodec:aac
1.3 2024-26-10
    - 10
    + 2
1.4 2024-26-10
    - 2
    + 5
1.5 2024-26-10
    - 5
    + 1
    # Unified Update
1.6 2024-26-10 - 1
    + 2
    + Version
1.7 2024-27-10
    - 'start "" "' from all O.S.s
    + 'start "UPDATE & DOWNLOAD" "' Win
1.8 2024-27-10
    - Start = ''
    - 1
    + Start = '"'
    + 2
1.9 2024-27-10
    + Check saved project
    - 1
    + 2
2.0 2024-27-10
    - "chmod +x " ..  MainPath
    + 'chmod +x "' ..  MainPath .. '"'
    # Ordered Variables
    - 2
    + 1
    + Apple Trial
2.3 2024-27-10
    # Linux execution correction
    + Credits
    # 2.1 and 2.2 just trials due issues with Linux and Apple
2.31 2024-28-10
    # Binaries directly form the source
2.32 2024-28-10
    - yt-dlp
    + yt-dlp_linux
2.4 2024-29-10
    # Adjusted header style for production
@tormyvancool
Copy link
Contributor Author

tormyvancool commented Nov 1, 2024

I've put all the Changelog because it's not present into the history of ReaPack but only in mine one.
And I would like it's all documented.

v2.5
Changed the path from 'Various' to 'Video'
@tormyvancool
Copy link
Contributor Author

it's now 2.5. Corrections made into the path from "Various" to "Video"

@tormyvancool
Copy link
Contributor Author

Sorry I remove the request. In 3 days I didn't get any feedback and today other scripts were merged so I'm not welcome.

@tormyvancool tormyvancool deleted the reapack.com_upload-1730472145832 branch November 4, 2024 20:30
@tormyvancool tormyvancool changed the title Release YOUTUBE Downloader v2.4 REMOVE IT Nov 4, 2024
@cfillion
Copy link
Member

cfillion commented Nov 4, 2024

so I'm not welcome

This one needs more work to review because it does shell command execution and ships third-party binaries (= more scrutiny warranted compared to a "normal" safe script). The one I merged earlier was an obviously OK 2-line update. Still haven't properly checked this one, but at a glance there were red flag bits that stood out:

         MainPath  = './yt-dlp_macos'
          Start = 'cd "' .. CallPath .. '" && chmod +x ' .. dlpMac .. ' && '
          os.execute('chmod +x "' ..  MainPath .. '"')

It does the chmod twice, but the second one happens before cd?... The Linux code path does chmod +x with double quotes but without any escaping.

Looks like GetRid gets rid of valid filename characters on macOS & Linux.

Then there's the sleep which freezes REAPER's main thread with a busy loop(!) instead of preferably polling every 30ms (reaper.defer)?

I assume that's there because on Windows the command is somehow spawned in the background? I haven't tried on Windows. yt-dlp doesn't behave that way on macOS and Linux.

The yt-dlp binaries are not pinned to a defined version, and supplied for the different OSes but without specifying the architecture? Is yt-dlp.exe 32-bit so it works on 64-bit Windows? The Linux one should probably come from the user's OS not the package, macOS one is ARM or x86_64 or both? (haven't checked yet, having to make sure before posting != fast review)

Finally there's a misindented line in the @about tag (metadata is indentation-sensitive) and multi-version changelog...

@tormyvancool
Copy link
Contributor Author

tormyvancool commented Nov 4, 2024

  • The Linux and Windows sections are working as supposed. They do the job.
  • MacOS has issues belonging to yt-dlp, this was recon by the devs themselves.
  • My code for MAC was a trial to workaround this strange behaviour but wasn't successful. I dropped it. It's seems to be due yt-dlp (discussion is in progress to understand)
  • GetRid() yes gets rid of characters I no want used at all. Nor for Windows/Linux or Mac whatsoever. They are not essential to the goal of the script. So no reasons to filter out S.O.s by S.O.s i will keep it as it is.
  • sleep() is essential and must be kept since yt-dlp returns false "end of process" to os.execute() causing the script to proceed further prematurely, when is shouldn't be at all. So the work around i put in place: polling each second if there is a change into the downloaded file size. And yes I tried also with "start /B /WAIT" and many other stuff ... no way. The workaround I chosen is the one it's working. I keep it as is.
    So if the filesize value doesn't change: then the process is finished and the script can continue. Otherwise: it waits for it.
  • I don't mind if Reaper """freezes""" (it doesn't really freeze) for few seconds. At the end it's needles to use it when it's downloading and importing the video on the timeline.
  • YT-DLP is delivered as I shown and I get the binaries form their GitHub directly, and they are always the most updated ones.
  • Although the script updates yt-dlp each time it's called before to download any video, I want that the user, once installs my script, starts with the most updated ones.
  • As I said it's working as supposed on Win and Linux but Apple has issues. So i will put apart Apple till the moment a solution is in place.

@cfillion
Copy link
Member

cfillion commented Nov 5, 2024

The Linux and Windows sections are working as supposed. They do the job.

"Works for me" is no excuse for lower quality. Obviously doing chmod twice, one of them broken (because before cd), isn't intended.

Sure the macOS/Linux code paths (double-quoted w/o escaping) likely works for most cases. But it will fail for some users because the full range of valid inputs is not considered. And the failure mode (arbitrary command injection) allows for catastrophic outcomes (even if unlikely in practice).

MacOS has issues belonging to yt-dlp, this was recon by the devs themselves.

I do use yt-dlp frequently on macOS but I can't comment on this specific issue you're referring to without without knowing what it is. But if it's broken then it shouldn't be in the script or users should be made aware of that...?

GetRid [...]

You're correct. From the quick glance I didn't notice it was just filtering user input (rather than existing filenames for the purpose of shell escaping).

sleep() is essential and must be kept [...] I don't mind if Reaper """freezes""" (it doesn't really freeze) for few seconds.

That's incorrect. The polling can (and should) be done without freezing REAPER for seconds. And yes making the main thread unresponsive for seconds/minutes/hours is a "freeze". A busy wait (100% CPU usage) makes it worse.

Also isn't the loop infinite if the output file fails to be created for whatever reason? The script does no error handling.

The file size can stop growing for more than a second before yt-dlp is done (eg. network speed drops temporarily halfway).

YT-DLP is delivered as I shown and I get the binaries

Missing the point which was the potential architecture mismatch? 32-bit vs 64-bit, ARM vs x86...

Anyway just checked: The Linux file is a python3 script = OK (if python3 is installed). The macOS one is a universal binary x86_64 and ARM64 = no 32-bit. The .exe is x86_64 = no 32-bit. (4% of ReaPack Windows downloads is 32-bit, 7% on macOS, but I do not know how many run on a 32-bit OS.) It's fine to not support all users but it must be documented/the package be made unavailable on unsupported OSes.

and they are always the most updated ones.

That means when a newer version breaks something the user can't easily downgrade to a known working version. (But it does have the advantage of not having to update & test the script regularly so I get your point.)

@tormyvancool
Copy link
Contributor Author

tormyvancool commented Nov 5, 2024

The Linux and Windows sections are working as supposed. They do the job.

"Works for me" is no excuse for lower quality. Obviously doing chmod twice, one of them broken (because before cd), isn't intended.

It works because tested also by the ones having linux, not only on my machine. hence the credits.
I can't test it on the whole world population (no one does or can do it)
if any glitch will occur THEN I will intervene to understand and if possible: solve it. Or versioning has no sense.
But for now it works as it should.

Sure the macOS/Linux code paths (double-quoted w/o escaping) likely works for most cases. But it will fail for some users because the full range of valid inputs is not considered. And the failure mode (arbitrary command injection) allows for catastrophic outcomes (even if unlikely in practice).

Then I will escape them. But strangely enough to open a quote like 'this is before the quote "' .. variable .. '" this is after the quote' it is even a solution suggested on LUA without counterindication.

sleep() is essential and must be kept [...] I don't mind if Reaper """freezes""" (it doesn't really freeze) for few seconds.

That's incorrect. The polling can (and should) be done without freezing REAPER for seconds. And yes making the main thread unresponsive for seconds/minutes/hours is a "freeze". A busy wait (100% CPU usage) makes it worse.

Also isn't the loop infinite if the output file fails to be created for whatever reason? The script does no error handling.

The file size can stop growing for more than a second before yt-dlp is done (eg. network speed drops temporarily halfway).

There is no way to do it differently.
Indeed in os.exec() when you give the string yt-dlp --update-to master -S vcodec:h264,res,acodec:aac <URL_of_the_video> -P "PATH" -o "TITLE.mp4" os.execute() goes ahead well before the process is terminated by yt-dlp

Even if I segregate the 2 processes (updates and download) the download starts before the update process is finished and the API to import the final video into the project reaper.InsertMedia(), is fired much before the download is finished.

It does so on all the platforms.

No way to capture the returns from os.exec() because it will return TRUE much before the process is finished. Hence: it is useless.

so by doing

local getthings = os.exec(variable_with_the_command_line_here)
if getthings == true then
     reaper.InsertMedia(whatsoever_here)
end

the reaper.InsertMedia() will be fired much before "variable_with_the_command_line_here" is finished.

I don't see any other way to verify that the download is really terminated.
Till now, that loop is the only one thing is really working.

EDIT - NOTE: another way is to test the presence of the partial file .. normally with .f137.mp4.part extension.
When it disappears, it means that the download is completed.
However it's always a loop

YT-DLP is delivered as I shown and I get the binaries

Missing the point which was the potential architecture mismatch? 32-bit vs 64-bit, ARM vs x86...

Not clue.

Anyway just checked: The Linux file is a python3 script = OK (if python3 is installed). The macOS one is a universal binary x86_64 and ARM64 = no 32-bit. The .exe is x86_64 = no 32-bit. (4% of ReaPack Windows downloads is 32-bit, 7% on macOS, but I do not know how many run on a 32-bit OS.) It's fine to not support all users but it must be documented/the package be made unavailable on unsupported OSes.

As I said, it will be documented because I will make a tutorial about where I say all what's needed.

I found only 4 detected OSes .. the Windows-ARM still not present "other" is for Linux
Hence I strongly limit the actions to these main 3 ones and with the availability of the available packages
image

So if for apple users the is not any solutions "sorry guys for you it's not possible to implement it".
And as you know I put the links into the @about or into the @Screenshots

@cfillion
Copy link
Member

cfillion commented Nov 5, 2024

I can't test it on the whole world population (no one does or can do it)

Well, sometimes testing the entire input possibility space is done (it's called fuzzing). 😉

But I just meant correctly handling the full input value range, not testing. Just standard programming.

Input is any non-null bytes representing a path to an existing file. Output is a shell script to be interpreted by sh, bash or cmd.exe. All inputs must map to a valid output.

Right now many inputs can make the shell behave unexpectedly (technically arbitrary command injection).

(cmd.exe has very different syntax rules than sh/bash! APIs such as os.execute are hard to get right and widely discouraged.)

There is no way to do it differently.

reaper.defer is the right way to do polling in a ReaScript. Not a busy loop.

os.execute() goes ahead well before the process is terminated by yt-dlp

This seems to only happen on Windows? I can't duplicate that behavior on macOS. os.execute blocks until yt-dlp is done and exits. Unless the command is launched as a background job using &.

Screencap

@tormyvancool
Copy link
Contributor Author

tormyvancool commented Nov 5, 2024

I can't test it on the whole world population (no one does or can do it)

Well, sometimes testing the entire input possibility space is done (it's called fuzzing). 😉

But I just meant correctly handling the full input value range, not testing. Just standard programming.

Input is any non-null bytes representing a path to an existing file. Output is a shell script to be interpreted by sh, bash or cmd.exe. All inputs must map to a valid output.

Right now many inputs can make the shell behave unexpectedly (technically arbitrary command injection).

(cmd.exe has very different syntax rules than sh/bash! APIs such as os.execute are hard to get right and widely discouraged.)

There is no way to do it differently.

reaper.defer is the right way to do polling in a ReaScript. Not a busy loop.

os.execute() goes ahead well before the process is terminated by yt-dlp

This seems to only happen on Windows? I can't duplicate that behavior on macOS. os.execute blocks until yt-dlp is done and exits. (Which ain't great UX, reaper.ExecProcess to launch in the background maybe?)

In the reality it was claimed also on Linux. But if you run my script in Reaper see what's happening just by commenting all the block

---------------------------------------------
-- UPDATE AND IMPORT VIDEO
---------------------------------------------

and just before it, set these PAIs in this way

os.execute(Video)
reaper.InsertMedia(Destination, 1)

Now I tried to comment the sleep() and it works fine as well
however to avoid it terminates due slow network as you highlighted, I can lopign also the test of the presence of a file with extension .f137.mp4.part .. if that file still exist and the size is not changed, is because the network is slow

but I will try now the repaer.defer() I never used before so no idea about syntaxis. The doc is very minimalistic. no examples on anything

@tormyvancool
Copy link
Contributor Author

ok I tried the reaper.defer() but it was a disaster since i have not clue how should I use it.
The documentation is too poor. Not any example available

@cfillion
Copy link
Member

cfillion commented Nov 5, 2024

There's multiple threads on the forum explaining how it works. Tons of scripts using it.

Pass a function to it and REAPER will call it 30ms later (nominally).

@tormyvancool
Copy link
Contributor Author

There's multiple threads on the forum explaining how it works. Tons of scripts using it.

Pass a function to it and REAPER will call it 30ms later (nominally).

I spotted this https://forums.cockos.com/showthread.php?p=2616483 but it created mor confusion than clarity.
On my case reaper.Defer(os.execute(blah)) ? ...

@cfillion
Copy link
Member

cfillion commented Nov 5, 2024

function checkWhetherItsFinished()
  if not reaper.file_exists('...part') then
    -- ...
  else
    reaper.defer(checkWhetherItsFinished)
  end
end

reaper.defer(checkWhetherItsFinished)

(I don't mind helping a bit, but this is a release repository, not ReaScript technical support...)

@tormyvancool
Copy link
Contributor Author

function checkWhetherItsFinished()
  if not reaper.file_exists('...part') then
    -- ...
  else
    reaper.defer(checkWhetherItsFinished)
  end
end

reaper.defer(checkWhetherItsFinished)

(I don't mind helping a bit, but this is a release repository, not ReaScript technical support...)

I appreciate it. Unfortunately it doesn't carry on the wanted result. And form what I see yt-delp is the more probable cause so I have to live with that loop
Please here the topic LINK to Cockos

@tormyvancool
Copy link
Contributor Author

function checkWhetherItsFinished()
  if not reaper.file_exists('...part') then
    -- ...
  else
    reaper.defer(checkWhetherItsFinished)
  end
end

reaper.defer(checkWhetherItsFinished)

(I don't mind helping a bit, but this is a release repository, not ReaScript technical support...)

under windows: solved. The 'start blah' had to be 'start /b /wait bla' the checking routing is automatically useless.
Reaper freezes while downloading but I avoid the polling etc ...

NOTE: I wasn't aware about these switches.

@tormyvancool
Copy link
Contributor Author

opened a different PR

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

Successfully merging this pull request may close these issues.

2 participants