Skip to content

Add uploadScreenshot to FirebaseAppDistributionTesterApiClient #3814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 15, 2022
Merged

Add uploadScreenshot to FirebaseAppDistributionTesterApiClient #3814

merged 5 commits into from
Jun 15, 2022

Conversation

lfkellogg
Copy link
Contributor

No description provided.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 14, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-appdistribution

    Overall coverage changed from 79.74% (da89e74) to 79.73% (666279e) by -0.01%.

    FilenameBase (da89e74)Merge (666279e)Diff
    FirebaseAppDistributionTesterApiClient.java87.37%86.87%-0.50%

Test Logs

Notes

  • Commit (666279e) is created by Prow via merging PR base commit (da89e74) and head commit (bf0fdfb).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/z8vwqe1w0C.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 14, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-appdistribution

    TypeBase (da89e74)Merge (666279e)Diff
    aar128 kB129 kB+1.00 kB (+0.8%)
    apk (aggressive)751 kB751 kB-36 B (-0.0%)
    apk (release)1.59 MB1.59 MB+624 B (+0.0%)

Test Logs

Notes

  • Commit (666279e) is created by Prow via merging PR base commit (da89e74) and head commit (bf0fdfb).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/4joBHO6cL0.html

Base automatically changed from lk/add-upload-screenshot to master June 14, 2022 20:01
kaibolay
kaibolay reviewed Jun 14, 2022
View reviewed changes
...in/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionTesterApiClient.java Outdated
String.format("upload/v1alpha/%s:uploadArtifact?type=SCREENSHOT", feedbackName);
ByteArrayOutputStream stream = new ByteArrayOutputStream();
screenshot.compress(Bitmap.CompressFormat.PNG, /* quality= */ 100, stream);
byte[] bytes = stream.toByteArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's pretty inefficient. You're first writing the bitmap to a ByteArrayOutputStream, then copy it into a byte array and then eventually send it to the HTTP connection's output stream. So the image ends up in memory multiple times. At least on low-end devices this might be noticible.

It might make sense to refactor the TesterHttpClient to either directly accept a Bitmap (although that would be kinda ugly breaking encapsulation) or a lambda to provide the data. Maybe the method would have a parameter Consumer<OutputStream>, so the labda would be stream -> screenshot.compress(Bitmap.CompressFormat.PNG, /* quality= */ 100, stream)

Also, currently the TesterHttpClient wraps the connections output stream in a GZIPOutputStream and sets the header Content-Encoding: gzip which would waste CPU trying to compress an already compressed bitmap. So the API exposed for the bitmap upload should not do that.

Sorry to not notice this in the previous PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I added a bespoke interface to allow us to handle the IOExceptions in the http client class though.

@kaibolay
Copy link
Contributor

please rebase

@lfkellogg lfkellogg force-pushed the lk/upload-screenshot-tester-api-client branch from 92ce55d to 6b48593 Compare June 14, 2022 20:32
kaibolay
kaibolay approved these changes Jun 14, 2022
View reviewed changes
...distribution/src/main/java/com/google/firebase/appdistribution/impl/TesterApiHttpClient.java
import java.io.UnsupportedEncodingException;
import java.util.HashMap;
import java.util.Map;
import java.util.zip.GZIPOutputStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you now never compress and data. I think that's probably OK, since all our API calls (except the screenshot upload which does it's own compression) tend to have pretty small payloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I figure this seems OK for now and we can always revisit.

@lfkellogg lfkellogg merged commit a2502fe into master Jun 15, 2022
@lfkellogg lfkellogg deleted the lk/upload-screenshot-tester-api-client branch June 15, 2022 13:53
@firebase firebase locked and limited conversation to collaborators Jul 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rachaprince rachaprince rachaprince approved these changes

@kaibolay kaibolay kaibolay approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lfkellogg @google-oss-bot @kaibolay @rachaprince