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

Issue with the ffmpeg patch #1

Closed
pukkandan opened this issue Aug 2, 2022 · 11 comments
Closed

Issue with the ffmpeg patch #1

pukkandan opened this issue Aug 2, 2022 · 11 comments

Comments

@pukkandan
Copy link

The current ffmpeg patch changes self.basename. But there are parts of the code that checks .basename == 'ffmpeg' to differentiate ffmpeg from avconv. A more appropriate patch would be:

diff --git a/yt_dlp/postprocessor/ffmpeg.py b/yt_dlp/postprocessor/ffmpeg.py
index 1f179c145..a098ff281 100644
--- a/yt_dlp/postprocessor/ffmpeg.py
+++ b/yt_dlp/postprocessor/ffmpeg.py
@@ -106,7 +106,7 @@ def _determine_executables(self):

         location = self.get_param('ffmpeg_location', self._ffmpeg_location.get())
         if location is None:
-            return {p: p for p in programs}
+            return {'ffmpeg': 'libffmpeg.bin.so', 'ffprobe': 'libffprobe.bin.so'}

         if not os.path.exists(location):
             self.report_warning(f'ffmpeg-location {location} does not exist! Continuing without ffmpeg')

Or, if you wish to support --ffmpeg-location for the so files, this should work instead

diff --git a/yt_dlp/postprocessor/ffmpeg.py b/yt_dlp/postprocessor/ffmpeg.py
index 45f7ab32e..606e46e01 100644
--- a/yt_dlp/postprocessor/ffmpeg.py
+++ b/yt_dlp/postprocessor/ffmpeg.py
@@ -102,11 +102,12 @@ def get_versions(downloader=None):
     _ffmpeg_to_avconv = {'ffmpeg': 'avconv', 'ffprobe': 'avprobe'}

     def _determine_executables(self):
+        bin_files = {'ffmpeg': 'libffmpeg.bin.so', 'ffprobe': 'libffprobe.bin.so'}
         programs = [*self._ffmpeg_to_avconv.keys(), *self._ffmpeg_to_avconv.values()]

         location = self.get_param('ffmpeg_location', self._ffmpeg_location.get())
         if location is None:
-            return {p: p for p in programs}
+            return bin_files

         if not os.path.exists(location):
             self.report_warning(f'ffmpeg-location {location} does not exist! Continuing without ffmpeg')
@@ -120,7 +121,7 @@ def _determine_executables(self):
             if basename in self._ffmpeg_to_avconv.keys():
                 self._prefer_ffmpeg = True

-        paths = {p: os.path.join(dirname, p) for p in programs}
+        paths = {p: os.path.join(dirname, so) for p, so in bin_files.items()}
         if basename:
             paths[basename] = location
         return paths

On a related note, do you think it makes sense for yt-dlp itself to handle the so files? The atomicparsley patch is simple enough to be directly added to yt-dlp without any issues. Some extra work will have to be done in case of ffmpeg, but I believe it is also not too hard

@xibr
Copy link
Owner

xibr commented Aug 3, 2022

Well, first we are using this repository to build yt-dlp for youtubedl-android which requires us to make a patch for it to work properly.

Before we switch to yt-dlp we were using youtubedl-lazy to build youtube-dl

I think we don't need to use --ffmpeg-location because the ffmpeg path cannot be changed, so first patch is better but I haven't tried it yet.

If this can be done it would be better if yt-dlp handles these files rather than making patches to these files whenever they are modified.

ffmpeg = libffmpeg.bin.so
ffprobe = libffprobe.bin.so
AtomicParsley = libatomicparsley.bin.so

@pukkandan
Copy link
Author

Alright, Let me see exactly how this can be implemented, and I'll get back to you.

For testing purposes, is there anywhere I can get the .so files (for windows/linux) without building it myself?

@xibr
Copy link
Owner

xibr commented Aug 3, 2022

If I understood correctly, We don't need to create .so files, because originally libffmpeg.bin.so is ffmpeg but we renamed it to libffmpeg.bin.so also for libffprobe.bin.so and libatomicparsley.bin.so

I have applied the patch

diff --git a/yt_dlp/postprocessor/ffmpeg.py b/yt_dlp/postprocessor/ffmpeg.py
index 45f7ab32e..8d1fc497c 100644
--- a/yt_dlp/postprocessor/ffmpeg.py
+++ b/yt_dlp/postprocessor/ffmpeg.py
@@ -106,7 +106,7 @@ def _determine_executables(self):
 
         location = self.get_param('ffmpeg_location', self._ffmpeg_location.get())
         if location is None:
-            return {p: p for p in programs}
+            return {'ffmpeg': 'libffmpeg.bin.so', 'ffprobe': 'libffprobe.bin.so'}
 
         if not os.path.exists(location):
             self.report_warning(f'ffmpeg-location {location} does not exist! Continuing without ffmpeg')

here we check if the patch work fine.

python3 -m yt_dlp --extract-audio --audio-format mp3 https://youtube.com/shorts/HKzOXY8-CZo
[youtube] HKzOXY8-CZo: Downloading webpage
[youtube] HKzOXY8-CZo: Downloading android player API JSON
[info] HKzOXY8-CZo: Downloading 1 format(s): 251
[download] Park Bo Gum's Beautiful Vocal Sing Western Sky By Lee Seung Chul _ Story Lirik Untuk Mantan #Shorts [HKzOXY8-CZo].webm has already been downloaded
[download] 100% of 421.90KiB
ERROR: Postprocessing: ffprobe and ffmpeg not found. Please install or provide the path using --ffmpeg-location

Run this command in /usr/bin (I have linux mint)

sudo cp ffmpeg libffmpeg.bin.so
sudo cp ffprobe libffprobe.bin.so

Now we have libffmpeg.bin.so and libffprobe.bin.so

Let's try again

python3 -m yt_dlp --extract-audio --audio-format mp3 https://youtube.com/shorts/HKzOXY8-CZo
[youtube] HKzOXY8-CZo: Downloading webpage
[youtube] HKzOXY8-CZo: Downloading android player API JSON
[info] HKzOXY8-CZo: Downloading 1 format(s): 251
[download] Park Bo Gum's Beautiful Vocal Sing Western Sky By Lee Seung Chul _ Story Lirik Untuk Mantan #Shorts [HKzOXY8-CZo].webm has already been downloaded
[download] 100% of 421.90KiB
[ExtractAudio] Destination: Park Bo Gum's Beautiful Vocal Sing Western Sky By Lee Seung Chul _ Story Lirik Untuk Mantan #Shorts [HKzOXY8-CZo].mp3
Deleting original file Park Bo Gum's Beautiful Vocal Sing Western Sky By Lee Seung Chul _ Story Lirik Untuk Mantan #Shorts [HKzOXY8-CZo].webm (pass -k to keep)

Success

@pukkandan
Copy link
Author

Is there any reason the filename is libatomicparsley.bin.so etc? Commonly, it is named libatomicparsley.so, no?

@xibr
Copy link
Owner

xibr commented Aug 8, 2022

You're right, the name should start with lib and end with .so as for why it was named like that maybe to tell the difference between libffmpeg.bin.so and libffmpeg.zip.so take a look at jniLibs/arm64-v8a

Anyway, I'm pretty sure if it's named like that then no problem
libffmpeg.so
libffprobe.so
libatomicparsley.so

I tested it and it works

diff --git a/yt_dlp/postprocessor/embedthumbnail.py b/yt_dlp/postprocessor/embedthumbnail.py
index 606d90d3d..4fb272d85 100644
--- a/yt_dlp/postprocessor/embedthumbnail.py
+++ b/yt_dlp/postprocessor/embedthumbnail.py
@@ -139,7 +139,7 @@ def run(self, info):
             if not success:
                 success = True
                 atomicparsley = next((
-                    x for x in ['AtomicParsley', 'atomicparsley']
+                    x for x in ['libatomicparsley.so', 'AtomicParsley', 'atomicparsley']
                     if check_executable(x, ['-v'])), None)
                 if atomicparsley is None:
                     self.to_screen('Neither mutagen nor AtomicParsley was found. Falling back to ffmpeg')

also for ffmpeg

diff --git a/yt_dlp/postprocessor/ffmpeg.py b/yt_dlp/postprocessor/ffmpeg.py
index 1f179c145..a098ff281 100644
--- a/yt_dlp/postprocessor/ffmpeg.py
+++ b/yt_dlp/postprocessor/ffmpeg.py
@@ -106,7 +106,7 @@ def _determine_executables(self):

         location = self.get_param('ffmpeg_location', self._ffmpeg_location.get())
         if location is None:
-            return {p: p for p in programs}
+            return {'ffmpeg': 'libffmpeg.so', 'ffprobe': 'libffprobe.so'}

         if not os.path.exists(location):
             self.report_warning(f'ffmpeg-location {location} does not exist! Continuing without ffmpeg')

If this is supported and added to yt-dlp I can make the change to youtubedl-android

xibr added a commit that referenced this issue Aug 8, 2022
@pukkandan
Copy link
Author

This is what I have in mind for the yt-dlp patch: yt-dlp/yt-dlp@master...pukkandan:yt-dlp-dev:features/detect-so

The situation with atomicparsley is trivial. I basically just copied over your patch.

For ffmpeg, youtube-dl-android will need to pass --ffmpeg-location libffmpeg.so to all yt-dlp invocations. If you did this previously, yt-dlp will detect ffmpeg, but not ffprobe. This patch ensures that libffprobe.so is automatically located from this command.

From my understanding, passing ffmpeg-location is a single line change here. If there is any difficulty with it, let me know

@xibr
Copy link
Owner

xibr commented Aug 9, 2022

This is what I have in mind for the yt-dlp patch: yt-dlp/yt-dlp@master...pukkandan:yt-dlp-dev:features/detect-so

It looks good, I have created yt_dlp.zip from your branch features/detect-so

For ffmpeg, youtube-dl-android will need to pass --ffmpeg-location libffmpeg.so to all yt-dlp invocations. If you did this previously, yt-dlp will detect ffmpeg, but not ffprobe.

I have set the ffmpe location yausername/youtubedl-android@master...xibr:youtubedl-android:set-ffmpeg-location

--ffmpeg-location /data/app/com.yausername.youtubedl_android_example-7wzfdHu3KxhrZtT8IZK1Mg==/lib/arm64/libffmpeg.bin.so I haven't changed the name yet but I will change it later.

This patch ensures that libffprobe.so is automatically located from this command.

You mean this patch [ffmpeg] Smarter detection of ffprobe filename?

I will test it and get back to you.

Finally we will need a file yt_dlp.zip like in this page https://github.com/xibr/ytdlp-lazy/releases/tag/2022.08.08 Which will be used directly from yt-dlp release page instead of using our repo, What do you think?

https://github.com/xibr/youtubedl-android/blob/set-ffmpeg-location/library/src/main/java/com/yausername/youtubedl_android/YoutubeDLUpdater.java

diff --git a/library/src/main/java/com/yausername/youtubedl_android/YoutubeDLUpdater.java b/library/src/main/java/com/yausername/youtubedl_android/YoutubeDLUpdater.java
index 74b79a1..5945c5d 100644
--- a/library/src/main/java/com/yausername/youtubedl_android/YoutubeDLUpdater.java
+++ b/library/src/main/java/com/yausername/youtubedl_android/YoutubeDLUpdater.java
@@ -22,7 +22,7 @@ class YoutubeDLUpdater {
     private YoutubeDLUpdater() {
     }
 
-    private static final String releasesUrl = "https://api.github.com/repos/xibr/ytdlp-lazy/releases/latest";
+    private static final String releasesUrl = "https://api.github.com/repos/yt-dlp/yt-dlp/releases/latest";
     private static final String youtubeDLVersionKey = "youtubeDLVersion";
 
     static UpdateStatus update(Context appContext) throws IOException, YoutubeDLException {

@pukkandan
Copy link
Author

I haven't changed the name yet but I will change it later.

Either name will work for ffmpeg/ffprobe. The .bin needs to be removed for only atomicparsley

You mean this patch [ffmpeg] Smarter detection of ffprobe filename?

I will test it and get back to you.

Yes. I can merge it as soon as you give ok

Finally we will need a file yt_dlp.zip like in this page 2022.08.08 (release) Which will be used directly from yt-dlp release page instead of using our repo, What do you think?

Can you not unzip the yt-dlp binary? It is really just the zip file with an added shebang

@xibr
Copy link
Owner

xibr commented Aug 11, 2022

Either name will work for ffmpeg/ffprobe. The .bin needs to be removed for only atomicparsley

That's good.

I tried it with the -v . command, I build it from [ffmpeg] Smarter detection of ffprobe filename
Note that ffprobe 4.2.1 has been found

[debug] Command-line config: ['-v', '--no-cache-dir', '--ffmpeg-location', '/data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/libffmpeg.bin.so']
[debug] Encodings: locale UTF-8, fs utf-8, pref UTF-8, out utf-8 (No ANSI), error utf-8 (No ANSI), screen utf-8 (No ANSI)
[debug] yt-dlp version 2022.08.08 [3157158f7] (zip)
[debug] Python 3.8.0 (CPython 64bit) - Linux-4.4.111-21427293-aarch64-with-libc (libc)
[debug] Checking exe version: /data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/libffmpeg.bin.so -bsfs
[debug] Checking exe version: /data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/libffprobe.bin.so -bsfs
[debug] exe versions: ffmpeg 4.2.1, ffprobe 4.2.1
[debug] Optional libraries: mutagen-1.45.1, sqlite3-2.6.0
[debug] Proxy map: {}

Usage: yt-dlp [OPTIONS] URL [URL...]

yt-dlp: error: You must provide at least one URL.
Type yt-dlp --help to see a list of all options.

And this was downloaded from the yt-dlp releases page

[debug] Command-line config: ['-v', '--no-cache-dir', '--ffmpeg-location', '/data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/libffmpeg.bin.so']
[debug] Encodings: locale UTF-8, fs utf-8, pref UTF-8, out utf-8 (No ANSI), error utf-8 (No ANSI), screen utf-8 (No ANSI)
[debug] yt-dlp version 2022.08.08 [3157158] (zip)
[debug] Python 3.8.0 (CPython 64bit) - Linux-4.4.111-21427293-aarch64-with-libc (libc)
[debug] Checking exe version: /data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/libffmpeg.bin.so -bsfs
[debug] Checking exe version: /data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/ffprobe -bsfs
[debug] Checking exe version: /data/app/com.yausername.youtubedl_android_example-L2GsFcscKE3dVP42DOcGRQ==/lib/arm64/avprobe -bsfs
[debug] exe versions: ffmpeg 4.2.1
[debug] Optional libraries: mutagen-1.45.1, sqlite3-2.6.0
[debug] Proxy map: {}

Usage: yt-dlp [OPTIONS] URL [URL...]

yt-dlp: error: You must provide at least one URL.
Type yt-dlp --help to see a list of all options.

Can you not unzip the yt-dlp binary? It is really just the zip file with an added shebang

Our code can not unzip the yt-dlp binary, here the code ZipUtils.java, But I have an idea and we run yt-dlp binary like this and we won't need the yt_dlp.zip file.

libpython.bin.so yt-dlp --version

The above output was running by this method.

@pukkandan
Copy link
Author

This is what I have in mind for the yt-dlp patch: yt-dlp/yt-dlp@master...pukkandan:yt-dlp-dev:features/detect-so

merged yt-dlp/yt-dlp@8420a4d

@xibr
Copy link
Owner

xibr commented Aug 11, 2022

Thank you for your interest in this and thank you for your hard work on yt-dlp.

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

No branches or pull requests

2 participants