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

GIFs from Gboard are sent as JPEG #6352

Open
JoeMcNuggets opened this issue Oct 7, 2024 · 13 comments · May be fixed by #6353
Open

GIFs from Gboard are sent as JPEG #6352

JoeMcNuggets opened this issue Oct 7, 2024 · 13 comments · May be fixed by #6353
Assignees
Labels
bug Something is not working

Comments

@JoeMcNuggets
Copy link

  • Android version: 14
  • Device: Pixel 7
  • Delta Chat version: 1.46.14
  • Expected behavior: gifs from google board are sent as gifs
  • Actual behavior: gifs from google board are sent as jpeg
  • Steps to reproduce the problem:
    • install gboard
    • open delta chat
    • select and send gif via gboard
@JoeMcNuggets JoeMcNuggets added the bug Something is not working label Oct 7, 2024
@adbenitez
Copy link
Member

hi @JoeMcNuggets thanks for reporting, I just tried the steps you described but the GIFs look perfectly fine/animated to me and they have the proper .gif file name and mime type:

image

@adbenitez
Copy link
Member

so maybe it is about the gboard version you are using? try upgrading, otherwise it is the specific GIFs files you are trying to use, then attach such GIF here so we can reproduce the problem, thanks in advance! 🙏

@JoeMcNuggets
Copy link
Author

JoeMcNuggets commented Oct 7, 2024 via email

@adbenitez
Copy link
Member

adbenitez commented Oct 7, 2024

thanks I could reproduce now, only if I attach it from Gboard (I searched for "goro majima") but if I directly attach the GIF file using the attachment selector then the gif is sent fine,

it seems to be related to the file size, since the GIF is too big it seems Delta Chat core is compressing it and converting it to JPEG, I don't know why it only happens when the image comes from the Gboard keyboard tho, it is likely related to:
https://stackoverflow.com/questions/48108889/gboard-enable-gif-insertion-on-edittext#48184426

@JoeMcNuggets
Copy link
Author

btw, sometimes Gboard says something like "this app does not support pasting gifs". Maybe it can help

@adbenitez
Copy link
Member

ok, after looking into this, it seems to be a bug in Delta Chat core:

  • when we receive the GIF file from keyboard we create a message with viewType DC_MSG_STICKER
  • core then determines the image is not a sticker (no transparent pixel) and convert it to DC_MSG_IMAGE viewType instead of DC_MSG_GIF
  • then since GIF is too big, the image compression logic is triggered and it recodes it to JPEG
  • even if GIF is small and not recoded, you can still see message type "Image" instead of Gif

cc @link2xt, @iequidoo

@adbenitez adbenitez transferred this issue from deltachat/deltachat-android Dec 20, 2024
@adbenitez
Copy link
Member

I think in general it is better to revert or improve the logic in this area, don't only check for transparency but if image is small enough (dimensions aren't super big ex. > 500px) then it is likely not a screenshot, and then it was intended to be sent as sticker, specially for gif, it should be always be sent as sticker as it is obviously not a screenshot

@iequidoo
Copy link
Collaborator

core then determines the image is not a sticker (no transparent pixel) and convert it to DC_MSG_IMAGE viewType instead of DC_MSG_GIF

Yes, because in the core DC_MSG_STICKER means "maybe sticker". There's Param::ForceSticker that UIs should set by calling Message::force_sticker() if they are sure it's a sticker. But this is only possible using the JSON-RPC API, not CFFI.

@adbenitez
Copy link
Member

core then determines the image is not a sticker (no transparent pixel) and convert it to DC_MSG_IMAGE viewType instead of DC_MSG_GIF

Yes, because in the core DC_MSG_STICKER means "maybe sticker". There's Param::ForceSticker that UIs should set by calling Message::force_sticker() if they are sure it's a sticker. But this is only possible using the JSON-RPC API, not CFFI.

maybe it was not clear enough: the problem is not the "maybe sticker" but if you are going to convert it out of sticker you need to check if it is actually a GIF type and not convert to IMAGE type always, otherwise then you apply image compression logic

@adbenitez
Copy link
Member

but anyways, if it is GIF type, you should preserve sticker viewtype since it is obviously not a screenshot, no need for force sticker setting in that case

@iequidoo
Copy link
Collaborator

iequidoo commented Dec 20, 2024

maybe it was not clear enough: the problem is not the "maybe sticker" but if you are going to convert it out of sticker you need to check if it is actually a GIF type and not convert to IMAGE type always, otherwise then you apply image compression logic

Good catch, but i'd say that if "ForceSticker" isn't set, we should rather convert it to Viewtype::Gif if it's actually GIF, and this is more straightforward to implement, just an additional check in prepare_msg_blob(). EDIT: I.e. absense of ForceSticker means that it may be converted to another viewtype using some heuristics.

@adbenitez
Copy link
Member

the thing is that the "maybe sticker" thing is pretty bad, if client is wanting to send file as sticker it is for reasons, the screenshot scenario is an edge case, not the rule, the client is not randomly adding images as stickers... they are added as stickers because it was selected by keyboard, core should not take the sticker view type so lightly, the problem you are trying to solve here is avoiding sending screenshots as sticker, other than that, if the UI is telling you it is a sticker, it is because we want it to be a sticker 😅

@iequidoo
Copy link
Collaborator

iequidoo commented Dec 20, 2024

[...] the problem you are trying to solve here is avoiding sending screenshots as sticker, other than that, if the UI is telling you it is a sticker, it is because we want it to be a sticker 😅

I agree that we should probably improve heuristics and don't give up Viewtype::Sticker so easily, but for this issue i suggest to improve prepare_msg_blob() which should look at the file extension and assign the Gif viewtype to the message. Screenshots shouldn't get the gif extension anyway, right?

EDIT: Finally i decided to leave Viewtype::Sticker if it's obviously not Image. Code-wise it's not more complex, but seems closer to what UIs expect.

@iequidoo iequidoo self-assigned this Dec 21, 2024
iequidoo added a commit that referenced this issue Dec 21, 2024
…tension (#6352)

Even if UIs don't call `Message::force_sticker()`, they don't want conversions of `Sticker` to
`Image` if it's obviously not an image, particularly, has non-image extension. Also UIs don't want
conversions of `Sticker` to anything other than `Image`, so let's keep the `Sticker` viewtype in
this case.
iequidoo added a commit that referenced this issue Dec 21, 2024
…tension (#6352)

Even if UIs don't call `Message::force_sticker()`, they don't want conversions of `Sticker` to
`Image` if it's obviously not an image, particularly, has non-image extension. Also UIs don't want
conversions of `Sticker` to anything other than `Image`, so let's keep the `Sticker` viewtype in
this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
3 participants