Feat: opus metadata encoding#12974
Conversation
This adds a simple timestamp to downloaded opus files in the form of a COMMENT tag.
This adds content-aware metadata tags to downloaded opus files. Previously, only a static string with a timestamp was added.
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/us/shandian/giga/service/DownloadManagerService.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
|
Hi, thanks for your review. You have some good points. But I disagree on the comment tag. I can see that the NewPipe version and download date do not add value for the user. But the source URL is very useful. For instance, when you want to see the music video of a song you downloaded, or when you want to see the video of a downloaded podcast. Encoding the URL into the metadata is the reason why I made this PR, and I don't see how this would obstruct the user. Would you prefer using a different tag? The Opus standard contains no standard tag for the source of an audio. That's why I chose the COMMENT tag. But the tags are formless, so one could use a custom one. See this RFC. |
This simplifies the code by not serializing and de-serializing the tags data, and also removes the DownloadManagerService.EXTRA_SOURCE field, which is always inferred from the StreamInfo.
|
I have added and tested your proposed changes, but I kept the bare URL in the comment tag for now. |
TobiGr
left a comment
There was a problem hiding this comment.
Thank you. LGTM, just a small nitpick
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/streams/OggFromWebMWriter.java
Outdated
Show resolved
Hide resolved
|
I have implemented this change, and it does not exceed the line limit. |
| final var metadata = new ArrayList<Pair<String, String>>(); | ||
| if (streamInfo != null) { | ||
| metadata.add(Pair.create("COMMENT", streamInfo.getUrl())); | ||
| metadata.add(Pair.create("GENRE", streamInfo.getCategory())); |
There was a problem hiding this comment.
The genre should only be set if getCategory() is not empty
There was a problem hiding this comment.
Line 357 of OggFromWebMWriter.java filters out empty tags. Have you tested this and found that this filter does not work? I have not yet come across a video without a category, so I might be wrong.
Feat: Downloading: Add opus audio metadata tags for title, author, date, and a comment tag with the originating URL This removes the DownloadManagerService.EXTRA_SOURCE field, which is always inferred from the StreamInfo.
|
I backported this to 0.28.1 |
What is it?
refactorbranchDescription of the changes in your PR
With this patch, downloaded .opus audio files will contain basic metadata (see example below). This is not a directly visible change, but comes in handy when you use downloaded audio with media players.
Here is the
ffprobeoutput of a downloaded file that shows all tags that were added.(On a side note, I thought about adding metadata encoding to m4a downloads as well. But after some research, I figured that this is a nontrivial change)
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence