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

Support Telegram animated stickers (tgs) format #1173

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

BenWiederhake
Copy link
Contributor

@BenWiederhake BenWiederhake commented Jul 17, 2020

This is half a fix for #874

This patch introduces two new config flags:

  • MediaConvertTgsToWebP
  • MediaConvertTgsToPNG

These need to be treated independently from the existing MediaConvertWebPToPNG flag because Tgs→WebP results in an animated WebP, and the WebP→PNG converter can't handle animated WebP files yet.

Furthermore, some platforms (like discord) don't even support animated WebP files, so the user may want to fall back to static PNGs (not APNGs).

The final reason why this is only half a fix is that this introduces an external dependency, namely lottie, to be installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp, calling lottie to convert it (this conversion may take several econds!), and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is unavailable, I can add yet another config option for that, if desired.

Here's a screenshot of Discord, with MediaConvertTgsToPNG=true:

Bildschirmfoto_2020-07-17_23-55-31

As this requires new text in the Wiki, I'll post some suggestions below.

@BenWiederhake
Copy link
Contributor Author

BenWiederhake commented Jul 17, 2020

Suggested wiki page: https://github.com/42wim/matterbridge/wiki/Settings

## MediaConvertTgsToWebP
Convert Tgs (Telegram animated sticker) images to WebP before upload.
This is independent of MediaConvertWebPToPNG. (This is useful because the result will be
an *animated* WebP file, and the WebP-to-PNG converter fails on animated WebP files.)
This requires the external dependency `lottie`, which can be installed like this:
`pip install lottie cairosvg`
https://github.com/42wim/matterbridge/issues/874

Setting: OPTIONAL, RELOADABLE \
Format: boolean \
Example: enable it

`MediaConvertTgsToWebP=true`

## MediaConvertTgsToPNG
Convert Tgs (Telegram animated sticker) images to PNG before upload.
This is useful when your bridge also contains platforms that do not support animated WebP files, like Discord.
This requires the external dependency `lottie`, which can be installed like this:
`pip install lottie cairosvg`
https://github.com/42wim/matterbridge/issues/874

Setting: OPTIONAL, RELOADABLE \
Format: boolean \
Example: enable it

`MediaConvertTgsToPNG=true`

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 17, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@BenWiederhake BenWiederhake marked this pull request as draft July 17, 2020 22:27
BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 17, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@BenWiederhake
Copy link
Contributor Author

I disagree with the linter.

  • switch { case … in maybeConvertTgs: Uhh, alright, if I must, but it looks weird to me.
  • logger can be github.com/stretchr/testify/assert.TestingT: I don't even understand what the error is about.
  • (lines 230 and 234) declaration of "err" shadows declaration at lines 224: I don't understand why it complains here but not usually.

@BenWiederhake BenWiederhake marked this pull request as ready for review July 17, 2020 23:20
@qaisjp
Copy link
Collaborator

qaisjp commented Jul 23, 2020

  • I don't understand why it complains here but not usually.

The reason it complains is because Go supports this:

func thisPanicsAndRecovers() (err error) {
	defer func() {
		message := recover()
		fmt.Println("Recovered from this panic:", message)
	}()

	err = errors.New("Some fake error")
	panic("aaaaa thisPanicsAndRecovers")
}

In the above code, the deferred function will get called when panic() is called. The deferred function recovers from the panic, and the outer function (thisPanicsAndRecovers) will return the error "Some fake error". We get this output:

Recovered from this panic: Some fake error

Now see this code:

func someError() error {
	return errors.New("Error from if statement")
}

func thisPanicsAndRecoversFailingLint() (err error) {
	defer func() {
		message := recover()
		fmt.Println("Recovered from this panic:", message)
	}()

	err = errors.New("Some fake error")
	if err := someError(); err != nil {
		panic("noooooo")
	}
	panic("aaaaa")
}

One might expect the if err := someError() to update the first err variable, and for this to be the output:

Recovered from this panic: Error from if statement

But it doesn't, the if statement creates a brand new err variable (it shadows the function's err variable that is auto-returned). We get this UNEXPECTED output:

Recovered from this panic: Some fake error


You can try it out here: https://play.golang.org/p/Y-OaeUn03-J

The solution is to call it writeErr and closeErr.

@qaisjp
Copy link
Collaborator

qaisjp commented Jul 23, 2020

logger can be github.com/stretchr/testify/assert.TestingT (interfacer)

Yeah, this one is weird. I have no idea.

@BenWiederhake
Copy link
Contributor Author

Thank you @qaisjp for the explanation, I totally forgot about the auto-returned variable :)

I'll update the code soon-ish.

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 25, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@BenWiederhake
Copy link
Contributor Author

Updated. (And rebased for convenience.)

Now the only remaining problem is this linter false-positive: ##[error]loggercan begithub.com/stretchr/testify/assert.TestingT (interfacer)
I'm going to ignore it.

@qaisjp
Copy link
Collaborator

qaisjp commented Jul 26, 2020

maybeCleanup is only used once, right? (In a defer.) Why not just defer an anonymous function? defer func() { ... }()

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 26, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@BenWiederhake
Copy link
Contributor Author

Nice, I wasn't aware of that syntax. Pushed it.

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 26, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@qaisjp
Copy link
Collaborator

qaisjp commented Jul 27, 2020

logger can be github.com/stretchr/testify/assert.TestingT (interfacer)

Yeah so this uses interfacer:

Deprecated: A tool that suggests interfaces is prone to bad suggestions, so its usefulness in real code is limited. This tool will remain available as a proof of concept, and for others to examine and learn from.

golangci/golangci-lint#541

And it looks like wim has just turned it off 2a41abb (1 hr after your last push)

So I guess just make another force push and golangci-lint should stop complaining.

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Jul 27, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@42wim
Copy link
Owner

42wim commented Aug 20, 2020

@BenWiederhake First off sorry for the delay and thank you for this PR.

This is a tricky one wrt the external dependency.

I'm fine with adding this feature with a few changes:

  • As a user normally will use either MediaConvertTgsToWebP or MediaConvertTgsToPNG, I think it's better to make this a string value. MediaConvertTgs="png" or MediaConvertTgs="webp", this also allows for other formats if ever necessary.
  • If the user has enabled this on startup and the dependency is not found, matterbridge should treat this as an error and exit on startup with a helpful message to the user. You can do this by adding a log.Fatal in the New() method of telegram for now (as we don't have support for returning an error there yet)

What do you think?

BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Aug 20, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
BenWiederhake added a commit to BenWiederhake/matterbridge that referenced this pull request Aug 20, 2020
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
This is half a fix for 42wim#874

This patch introduces two new config flags:
- MediaConvertTgsToWebP
- MediaConvertTgsToPNG

These need to be treated independently from the existing
MediaConvertWebPToPNG flag because Tgs→WebP results in an
*animated* WebP, and the WebP→PNG converter can't handle
animated WebP files yet.

Furthermore, some platforms (like discord) don't even support
animated WebP files, so the user may want to fall back to
static PNGs (not APNGs).

The final reason why this is only half a fix is that this
introduces an external dependency, namely lottie, to be
installed like this:

$ pip3 install lottie cairosvg

This patch works by writing the tgs to a temporary file in /tmp,
calling lottie to convert it (this conversion may take several seconds!),
and then deleting the temporary file.
The temporary file is absolutely necessary, as lottie refuses to
work on non-seekable files.
If anyone comes up with a reasonable use case where /tmp is
unavailable, I can add yet another config option for that, if desired.

I will propose new text for the Wiki in the PR for this patch.
(Should be 42wim#1173 or so.)
@BenWiederhake
Copy link
Contributor Author

Updated, and rebased for convenience.

Suggested wiki page: https://github.com/42wim/matterbridge/wiki/Settings

## MediaConvertTgs
Convert Tgs (Telegram animated sticker) images to some other file format before upload.
This requires the external dependency `lottie`, which can be installed like this:
`pip install lottie cairosvg`
https://github.com/42wim/matterbridge/issues/874

Setting: OPTIONAL, RELOADABLE \
Format: string \
Values: `"png"` (still-image), `"webp"` (animated webp)
Example: Convert to png because a gateway is involved that doesn't even understand animated webp:

`MediaConvertTgs="png"`

@42wim
Copy link
Owner

42wim commented Aug 23, 2020

LGTM, thanks!

@42wim 42wim merged commit b2af76e into 42wim:master Aug 23, 2020
@42wim
Copy link
Owner

42wim commented Aug 23, 2020

I've added your change to the wiki

@42wim 42wim added this to the 1.18.1 milestone Aug 23, 2020
42wim pushed a commit that referenced this pull request May 13, 2021
this was buried and wanted to bring it up in the config

Convert Tgs (Telegram animated sticker) images to PNG before upload.
This is useful when your bridge also contains platforms that do not support animated WebP files, like Discord.
This requires the external dependency `lottie`, which can be installed like this:
`pip install lottie cairosvg`
#874

#1173
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.

3 participants