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

Only escape backslashes, single quotes, and newlines when writing ffmetadata #1161

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

superbonaci
Copy link
Contributor

@superbonaci superbonaci commented Jul 24, 2024

this documentation is correct:
https://ffmpeg.org/ffmpeg-utils.html#Quoting-and-escaping

this is outdated:
https://ffmpeg.org/ffmpeg-formats.html#metadata

Escaping only backslash \\ is enough but single quotes can be escaped no problem.

@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

Did you test this change?

@superbonaci
Copy link
Contributor Author

superbonaci commented Jul 24, 2024

yes, you can test with some videos having those letters. These are without the PR, as is working right now:

The first title is in Twitch

(*$&@(*!$&(!*@&$)(&!@(*&$(*)!@&(*) ALL NIGHT (*&()#()!$^&(@#$^*&@#!^$*&(#$^@&*#($^!*(& COOL *&^#()*$&!)_@$*_)@#*()$* DRAMA N STUFF (*&$(*&!@

But in metadata in video

(*$&@(*!$&(!*@&$)(&!@(*&$(*)!@&(*) ALL NIGHT (*&()\#()!$^&(@\#$^*&@\#!^$*&(\#$^@&*\#($^!*(& COOL *&^\#()*$&!)_@$*_)@\#*()$* DRAMA N STUFF (*&$(*&!@ (2203127860)

video

The second title in Twitch

[DROPS] |🎰MAX INVENTORY STRONGBOX🎰BAD CV = STANDARD PULL🎰Summer Event -> Challenge Account | !challenge !faq !youtube

but in video is

[DROPS] |🎰MAX INVENTORY STRONGBOX🎰BAD CV \= STANDARD PULL🎰Summer Event -> Challenge Account | !challenge !faq !youtube (2204591823)

third video is

Highlight: JnSpit's Real Talk

third title in video (the ' is not escaped but if you escape it works too, no idea what is best)

Highlight: JnSpit's Real Talk (2200327009)

fourth video in twitch

FEELING ALIL SICK, MIGHT BE A SHORT CHILL STREAM! (;-; | !merch !tts !game !discord !charity !dnd !collab

in video

 FEELING ALIL SICK, MIGHT BE A SHORT CHILL STREAM! (\;-\; | !merch !tts !game !discord !charity !dnd !collab (2205834888)

fifth video original title is

KSM/KSH/Portals with Viewers | ❗vote for @Dillisan - MMO Awards: VIDEO OF THE YEAR \o/

In video is

KSM/KSH/Portals with Viewers | ❗vote for @Dillisan - MMO Awards: VIDEO OF THE YEAR \o/ (2202575617)

sixth video original title is

Questing \n money runs

In video is

Questing \n money runs (2172757417)

@superbonaci
Copy link
Contributor Author

The missing video is the one with \n in title or description, let me know if you find one

@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

The missing video is the one with \n in title or description, let me know if you find one

https://www.twitch.tv/videos/2166590043
Description is completely broken because the \ns were not escaped.

Copy link
Collaborator

@ScrubN ScrubN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment somewhere, preferably above the method name, explaining that the documentation is outdated and = ; # do not need to be escaped.

TwitchDownloaderCore/Tools/FfmpegMetadata.cs Show resolved Hide resolved
@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

Also, have you reported this to the FFmpeg team yet?

@superbonaci
Copy link
Contributor Author

superbonaci commented Jul 24, 2024

I've added the 6 videos each one to see is each character is escaped or not. The \n does not mean escape, you have to use \ at the end like you do, like video 2172757417.

The downloaded metadata.txt is:

;FFMETADATA1
title=Questing \\n money runs (2172757417)
artist=gaptyx
date=2024
comment=Originally aired: 2024-06-15 15:04:17Z\
Video id: 2172757417\
Views: 6
[CHAPTER]
TIMEBASE=1/1000
START=0
END=120000
title=Escape from Tarkov

but if you want to put the title in two lines the \n does not mean anything, will be converted to n. Twitch already sends the\u000A you do not have to convert also \n to \u000A, and should look like this:

;FFMETADATA1
title=Questing \\n money runs (2172757417)\
title continues\
last title
artist=gaptyx
date=2024
comment=Originally aired: 2024-06-15 15:04:17Z\
Video id: 2172757417\
Views: 6
[CHAPTER]
TIMEBASE=1/1000
START=0
END=120000
title=Escape from Tarkov

The issue is known: Chaotic escaping rules

@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

I did not say n needs to be escaped, \u000A (the LINE_FEED constant) needs to be escaped.

@superbonaci
Copy link
Contributor Author

Already returned.

@superbonaci superbonaci requested a review from ScrubN July 24, 2024 22:37
@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

Can you add the comment?

@superbonaci
Copy link
Contributor Author

superbonaci commented Jul 24, 2024

The missing video is the one with \n in title or description, let me know if you find one

https://www.twitch.tv/videos/2166590043 Description is completely broken because the \ns were not escaped.

yes that's a multi line description, but i meant that the description included the \n literal like this has in the title https://www.twitch.tv/videos/2172757417

@ScrubN
Copy link
Collaborator

ScrubN commented Jul 24, 2024

yes that's a multi line description, but i meant that the description included the \n literal like this has in the title https://www.twitch.tv/videos/2172757417

Yes, that will stay as the literal \n

@superbonaci
Copy link
Contributor Author

comment done.

Copy link
Collaborator

@ScrubN ScrubN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this

@ScrubN ScrubN changed the title Only escape backslashes and single quotes in SanitizeKeyValue Only escape backslashes, single quotes, and newlines when writing ffmetadata Jul 24, 2024
@ScrubN ScrubN merged commit 4c65eb8 into lay295:master Jul 24, 2024
@superbonaci superbonaci deleted the ;FFMETADATA1 branch July 24, 2024 23:11
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