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

Port HTTPRequest gzip compression to 3.4 #48651

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Conversation

tavurth
Copy link
Contributor

@tavurth tavurth commented May 11, 2021

Tests are finally passing 🎉

#48581

Edit: Backport of #38944

@tavurth tavurth requested review from a team as code owners May 11, 2021 17:52
@tavurth tavurth changed the title Work on porting HTTPRequest compression to 3.3 Port HTTPRequest gzip compression to 3.4 May 11, 2021
@Calinou Calinou added this to the 3.4 milestone May 11, 2021
@tavurth
Copy link
Contributor Author

tavurth commented May 11, 2021

@Wavesonics I hope I ported your code faithfully, thank you for the original PR!

@Wavesonics
Copy link
Contributor

Nice, it looks good to me!

@akien-mga akien-mga requested review from a team and removed request for a team May 14, 2021 07:45
Fix doc issues

Use memcpy

Bind RESULT_BODY_DECOMPRESS_FAILED

Docs update
@tavurth
Copy link
Contributor Author

tavurth commented Jun 17, 2021

@Calinou sorry to ping you on this, is there something else I should do to push for this in the 3.4 release?

Excited to see this feature speeding up my API calls :)

@akien-mga akien-mga merged commit c7f27f1 into godotengine:3.x Jun 18, 2021
@akien-mga
Copy link
Member

Thanks!

@Faless
Copy link
Collaborator

Faless commented Oct 13, 2021

This causes a few regressions including #53085 so it needs to be reverted. The original PR on master #38944 is left for now but will need work if we want to keep in 4.0 (or will need revert too).

Issues:

  • Some platform/HTTPClient might not allow those headers (HTML5)
  • Response is not decompressed when saved to file.
  • We need facilities to decompress a stream (i.e. not decompress all in memory but in chunks to file).

@Wavesonics
Copy link
Contributor

I can put some time in on the master implementation to get it fixed up

@RPicster
Copy link
Contributor

RPicster commented Dec 7, 2021

Will this feature be looked into again?
It would be great to have in 3.x.
Could it be eventually solved by not making it automatic but by a parameter?

@tavurth
Copy link
Contributor Author

tavurth commented Dec 8, 2021

@RPicster if we have a working master implementation then this should be pretty trivial to port back to 3.x again.

@Wavesonics
Copy link
Contributor

I'm taking a look at what needs to be done here,

  • Some platform/HTTPClient might not allow those headers (HTML5) can you elaborate? Is this a problem with the web export? What would a solution look like? Disable the option to include the header on that export or something?
  • Response is not decompressed when saved to file. easy enough, I can implement that
  • We need facilities to decompress a stream (i.e. not decompress all in memory but in chunks to file). I think this should be do-able using the new stream decompression I added for this already, I'll look into it.

Other than these 3 issues is there anything else that is needed to consider this fixed?

@Wavesonics
Copy link
Contributor

#55946 addresses the 2nd two points listed above, we can now stream decompress a file to another file (though that is not exposed through GDScript yet.) and that is then used to decompress the downloaded to file option in HttpRequest.

I don't have any idea how we deal with HTML5 as a platform in Godot, so maybe someone could clue me in. It sort of sounds like on HTML5 exports, we should just turn off the "Accept Gzip header" option under the hood or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants