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

Mobile upload file transfer speed is widely incorrect #11127

Open
1 of 3 tasks
jadenquinn opened this issue Jul 15, 2024 · 10 comments
Open
1 of 3 tasks

Mobile upload file transfer speed is widely incorrect #11127

jadenquinn opened this issue Jul 15, 2024 · 10 comments

Comments

@jadenquinn
Copy link

The bug

Basically the speed the app shows the file being uploading at (example: 38.45MB/s), is almost always completely incorrect from what it actually is, verified very easily by eye. Sometimes it even jumps to over 300MB/s, which of course isn’t actually anywhere close to what’s tx-ing.
IMG_6805
IMG_6804
IMG_6803
IMG_6802
IMG_6801
IMG_6800
IMG_6799

IMG_6797

![Uploading IMG_6798.png…]
IMG_6795
IMG_6796

The OS that Immich Server is running on

MacOS 14.5

Version of Immich Server

Latest (as of 2 hours ago)

Version of Immich Mobile App

Latest (as of 1 hour ago)

Platform with the issue

  • Server
  • Web
  • Mobile

Your docker-compose.yml content

N/A

Your .env content

N/A

Reproduction steps

Run immich on iOS and start a manual backup.

Relevant log output

No response

Additional information

This is an issue with the Immich mobile app I think.

@alextran1502
Copy link
Contributor

It is calculated based on the moving average value and timer, so it can be inaccurate if your system's IO speed is slower. We are open to suggestions on how to calculate it better

Here is relevant PR #7760

@jadenquinn
Copy link
Author

It is calculated based on the moving average value and timer, so it can be inaccurate if your system's IO speed is slower. We are open to suggestions on how to calculate it better

Here is relevant PR #7760

I've never ran into any app that does what this is doing, so... I'd imagine doing what everyone else is doing for calculations would be better.

@alextran1502
Copy link
Contributor

Do you have suggestions on how other apps are doing the calculation?

@othyn
Copy link
Contributor

othyn commented Jul 22, 2024

The original implementation ran a simple calculation of the amount of data uploaded in the last x period, per period, which is how its always done. That is the industry norm, as its just 'data uploaded over time', calculated reactively to the current upload. Using other OSS apps as reference for how they handled the upload speed calculation, and confirming its the way to go about things. But as some uploads completed in under x period, no speed would be displayed. Shrinking the period also didn't help due to the way the upload library handles feeding back the data uploaded during the period.

It was later changed to provide an average over multiple uploads/samples via a buffer, as a change requested from the team against the PR before merge, in an effort to try and 'even out' the displayed value.

If you have a read through the PR, you'll see the back-and-forth discussion on finding a reasonable implementation strategy for this value, and all the problems which arose whilst doing so (all of the above was covered, and you can see the reasoning as to why certain decisions were made).

I wanted to submit a new PR to lower the buffer/sample size from 10 down to ~3, to make the value more reactive. The other option is to increase the buffer/sample size from 10 to ~100 to even out the bumps from smaller uploads (more on that later). As as I've been using the implementation, I've found the same, in the upload speed bouncing around.

I think the trigger is small uploads that happen in <1-2 seconds, multiple in a given x polling period for the speed calculation, for some reason they cause erratic values. Larger/longer uploads appear to stabilise the value and provide a more realistic representation of the upload speed. Windows isn't the best example of an implementation of this, but theirs classically always overshoots its transfer speed reporting first, before then settling into something more realistic (drive hardware, file system, file size, etc. depending).

We're also working within the bounds of the library reporting the data uploaded over the last period, which there are some restrictions with.

If you have better implementation suggestions for the calculation, I'm all ears!

@MattKayDev
Copy link

I'm seeing the same issue while uploading larger files like recordings

@jadenquinn
Copy link
Author

jadenquinn commented Sep 16, 2024

The original implementation ran a simple calculation of the amount of data uploaded in the last x period, per period, which is how its always done. That is the industry norm, as its just 'data uploaded over time', calculated reactively to the current upload. Using other OSS apps as reference for how they handled the upload speed calculation, and confirming its the way to go about things. But as some uploads completed in under x period, no speed would be displayed. Shrinking the period also didn't help due to the way the upload library handles feeding back the data uploaded during the period.

It was later changed to provide an average over multiple uploads/samples via a buffer, as a change requested from the team against the PR before merge, in an effort to try and 'even out' the displayed value.

If you have a read through the PR, you'll see the back-and-forth discussion on finding a reasonable implementation strategy for this value, and all the problems which arose whilst doing so (all of the above was covered, and you can see the reasoning as to why certain decisions were made).

I wanted to submit a new PR to lower the buffer/sample size from 10 down to ~3, to make the value more reactive. The other option is to increase the buffer/sample size from 10 to ~100 to even out the bumps from smaller uploads (more on that later). As as I've been using the implementation, I've found the same, in the upload speed bouncing around.

I think the trigger is small uploads that happen in <1-2 seconds, multiple in a given x polling period for the speed calculation, for some reason they cause erratic values. Larger/longer uploads appear to stabilise the value and provide a more realistic representation of the upload speed. Windows isn't the best example of an implementation of this, but theirs classically always overshoots its transfer speed reporting first, before then settling into something more realistic (drive hardware, file system, file size, etc. depending).

We're also working within the bounds of the library reporting the data uploaded over the last period, which there are some restrictions with.

If you have better implementation suggestions for the calculation, I'm all ears!

I don't see how that method could do what I am observing. W. T. F.
It can be so off that the amount it says it's doing per second can be twice as big as the file its uploading to begin with, and that's after a couple seconds of it uploading it. In reality it hovers around 30MB/s, but sometimes it says 200!? There's obviously some actual bug in that way that method up there is implemented. Maybe it has something to do with why my phone gets so unusably hot when uploading stuff to the Immich server.

The better implementation to suggest is... just fix the one that's already there. It's very much broken. I can't really read anything but Java so I can't say what, and I hate math, but something is very wrong with what it's doing.

@othyn
Copy link
Contributor

othyn commented Sep 16, 2024

Sorry, but there is no need to be this rude about a potential issue in an upload speed indicator in a photo app. If you'd care to actually be constructive instead of combative, put your time to better use, review the code and come up with proposals to fix the issue and lose the attitude, its unnecessary.

Use it as a learning opportunity to give Dart a go, to brush up your maths skills and help out. What do you think we all do? We're all here to volunteer our time to learn ourselves when we face roadblocks, bugs and issues. To build software that people will use and enjoy. Not do as thine commands.

Either be kind, constructive and help, or please keep comments to yourself.

Its also worth pointing out that on mine and many others devices, the upload speed indication works absolutely fine and is in alignment with the expected network throughput, which makes problems like this hard to debug and understand. These things take time.


Now, if we actually constructively try to discuss the potential issue, instead of addressing the absurdity of some of your other spurious claims, the calculation behind it is very simple and relies on the figures provided by the upload library to calculate (minimum poll duration is 1 second, anything lower than that provides unusable data, as originally I'd wanted it to be inline with data sent to the server (so on the scale of millisecond updates instead of seconds)), its the only way you can calculate things like this; bytes sent over time period.

Firstly, calculate the time interval in which has occurred between now and then.

duration = now - lastUpdateTime

Secondly, compute the current speed, using the bytes uploaded value by the upload library and the seconds duration between now and the last calculation.

There is some nuance, as the upload library tracks total bytes sent, so there has to be a prior calculation to work out the bytes uploaded across the period first.

(sent - lastSentBytes) / duration.inSeconds

This is the upload speed.

Finally, the value is added to the 'pool', and we grab the average over the 'pool' to ease out the UI.

lastUploadSpeeds.average.abs()

This wasn't in the original implementation, but it was requested to be added by the maintainers to make the UI more consistent across multiple uploads at the loss of some accuracy.

Now the way I see it, stepping through the code with a fresh head and putting my 'PR reviewer' hat on, I can see a few potential issues, one of which I raised originally when this change was requested;

  1. sent vs. lastSentBytes
    • The nature of these variables sticks out to me
    • sent will potentially be for a prior file when the calculation bridges uploads
    • lastSentBytes will potentially be for a prior file when the calculation bridges uploads
    • This could skew the upload speed calculation as the bytes sent could be massive even for a relationally small file, as its potentially using a prior large file within those same values
    • Take the example of within a polling period (which is 1 second), a large several 100MB file finishes uploading and a small several 100KB image begins uploading. In that one singular polling period, the sent bytes could be (on a clean gigabit LAN) in the range of around 20MB-60MB ish
    • This is as the upload library is purely keeping count on the amount of bytes sent as a sum total, it has no context over what this file and that file are, just data that its uploaded
    • As sent and lastSentBytes are potentially never reset to zero once a prior file uploads, it could carry over the data value, which could potentially skew the upload speed
    • More research is needed into the data lifecycle of the component and the upload library as to the nature of these variables, as a lot of this has been lost from memory of playing around with them when building out the feature originally many moons ago, as the sent bytes value and retained 'last' values could be flushed when a new file is spawned, making this a moot point
  2. The timer interval may not be called to update the upload speed value if the upload or consecutive uploads occur in under the 1 second period, meaning you can potentially go several uploads and never receive an upload speed. Not really sure how to approach this, as the upload speed polling would have to occur on a separate process/thread to the upload, so that the speed and dip in and take values over the duration of the entire upload process. Although this really does need testing, as I'm not sure.
  3. One I raised originally, as my original implementation just used the live upload speed instead of the requested average addition from the maintainers. But, as the non-average implementation would 'reset' the speed each upload (as it starts the calculation from a fresh and needs to wait 1 second to gather a period to calculate across) and would therefor be immune to such a bug, they wanted the value to 'persist' to keep the UI consistent and display a value across multiple uploads.
    • The size of the 'average pool' can skew the results until the pool is refilled with data relating to the current upload and therefor can calculate a more accurate average based on solely the upload for the current file
    • We can shrink the pool, to something like 2-3 samples, which will make the speed more reactive to the current file and allow for the requirement that the UI shows a value that 'spans' an upload so the value is never empty, but keeps it more accurate to the current upload, or bin it entirely and simply cache the last value to persist it. However there was originally a desire not to do this from the original maintainers to make a smoother experience
    • We can expand the pool, to something like 100-200 samples, to average out the speed across the upload session as a whole and absorb values that are out-of-bounds, although I'd rather figure out why those values pop up to begin with and fix the root of the problem

I'll need to re-setup a development environment for this and debug it locally, perhaps tonight.


Also, if you're going to reply, please only reply with something constructive and approach it with a genuine desire to help. Anything else will just be ignored, we're not here to deal with attitude, we're here to write software.

@jadenquinn
Copy link
Author

jadenquinn commented Sep 19, 2024

Sorry, but there is no need to be this rude about a potential issue in an upload speed indicator in a photo app. If you'd care to actually be constructive instead of combative, put your time to better use, review the code and come up with proposals to fix the issue and lose the attitude, its unnecessary.

Use it as a learning opportunity to give Dart a go, to brush up your maths skills and help out. What do you think we all do? We're all here to volunteer our time to learn ourselves when we face roadblocks, bugs and issues. To build software that people will use and enjoy. Not do as thine commands.

Either be kind, constructive and help, or please keep comments to yourself.

Its also worth pointing out that on mine and many others devices, the upload speed indication works absolutely fine and is in alignment with the expected network throughput, which makes problems like this hard to debug and understand. These things take time.

Now, if we actually constructively try to discuss the potential issue, instead of addressing the absurdity of some of your other spurious claims, the calculation behind it is very simple and relies on the figures provided by the upload library to calculate (minimum poll duration is 1 second, anything lower than that provides unusable data, as originally I'd wanted it to be inline with data sent to the server (so on the scale of millisecond updates instead of seconds)), its the only way you can calculate things like this; bytes sent over time period.

Firstly, calculate the time interval in which has occurred between now and then.

duration = now - lastUpdateTime

Secondly, compute the current speed, using the bytes uploaded value by the upload library and the seconds duration between now and the last calculation.

There is some nuance, as the upload library tracks total bytes sent, so there has to be a prior calculation to work out the bytes uploaded across the period first.

(sent - lastSentBytes) / duration.inSeconds

This is the upload speed.

Finally, the value is added to the 'pool', and we grab the average over the 'pool' to ease out the UI.

lastUploadSpeeds.average.abs()

This wasn't in the original implementation, but it was requested to be added by the maintainers to make the UI more consistent across multiple uploads at the loss of some accuracy.

Now the way I see it, stepping through the code with a fresh head and putting my 'PR reviewer' hat on, I can see a few potential issues, one of which I raised originally when this change was requested;

1. `sent` vs. `lastSentBytes`
   
   * The nature of these variables sticks out to me
   * `sent` will _potentially_ be for a prior file when the calculation bridges uploads
   * `lastSentBytes` will _potentially_ be for a prior file when the calculation bridges uploads
   * This could skew the upload speed calculation as the bytes sent could be massive even for a relationally small file, as its potentially using a prior large file within those same values
   * Take the example of within a polling period (which is 1 second), a large several 100MB file finishes uploading and a small several 100KB image begins uploading. In that one singular polling period, the sent bytes _could_ be (on a clean gigabit LAN) in the range of around 20MB-60MB ish
   * This is as the upload library is purely keeping count on the amount of bytes sent as a sum total, it has no context over what this file and that file are, just data that its uploaded
   * As `sent` and `lastSentBytes` are potentially never reset to zero once a prior file uploads, it could carry over the data value, which could potentially skew the upload speed
   * More research is needed into the data lifecycle of the component and the upload library as to the nature of these variables, as a lot of this has been lost from memory of playing around with them when building out the feature originally many moons ago, as the sent bytes value and retained 'last' values could be flushed when a new file is spawned, making this a moot point

2. The timer interval may not be called to update the upload speed value if the upload or consecutive uploads occur in under the 1 second period, meaning you can potentially go several uploads and never receive an upload speed. Not really sure how to approach this, as the upload speed polling would have to occur on a separate process/thread to the upload, so that the speed and dip in and take values over the duration of the entire upload process. Although this really does need testing, as I'm not sure.

3. One I raised originally, as my original implementation just used the live upload speed instead of the requested average addition from the maintainers. But, as the non-average implementation would 'reset' the speed each upload (as it starts the calculation from a fresh and needs to wait 1 second to gather a period to calculate across) and would therefor be immune to such a bug, they wanted the value to 'persist' to keep the UI consistent and display a value across multiple uploads.
   
   * The size of the 'average pool' can skew the results until the pool is refilled with data relating to the current upload and therefor can calculate a more accurate average based on solely the upload for the current file
   * We can shrink the pool, to something like 2-3 samples, which will make the speed more reactive to the current file and allow for the requirement that the UI shows a value that 'spans' an upload so the value is never empty, but keeps it more accurate to the current upload, or bin it entirely and simply cache the last value to persist it. However there was originally a desire not to do this from the original maintainers to make a smoother experience
   * We can expand the pool, to something like 100-200 samples, to average out the speed across the upload session as a whole and absorb values that are out-of-bounds, although I'd rather figure out why those values pop up to begin with and fix the root of the problem

I'll need to re-setup a development environment for this and debug it locally, perhaps tonight.

Also, if you're going to reply, please only reply with something constructive and approach it with a genuine desire to help. Anything else will just be ignored, we're not here to deal with attitude, we're here to write software.

Not meaning to be rude, just saying exactly what I mean... and.. I have proof that it really is as bad as I say it is, I'm not out of line. I'm very direct, that's all. Attitude has absolutely nothing to do with any of this.
I will not ever code. Sorry. Just not something I'm interested in. A surprising number of people genuinely keep telling me I should, but coding genuinely pisses me off, so I've learned to leave that to more skilled folks.

I don't need to understand code to know that I have been uploading things from my phone for a decade, and I've never seen any other app have this bug. Not that I know how to fix it, but that a fix certainly does exist and isn't hyper complicated. I won't try to understand most of what you typed there.. mainly because it's way over my head, but yea whatever that is, it's breaking, and there is a fix because every other app on my phone doesn't do this, so like I've been saying, just do what they do.

Thanks for the in-depth-ness of this though, I'm hoping someone who does understand all of it can fix this bug, and that will definitely help them in their quest.

@othyn
Copy link
Contributor

othyn commented Sep 20, 2024

I understand you're frustrated, but the tone and approach you're taking here aren't constructive. It's possible to be direct without being dismissive or rude, and unfortunately, your responses come across as both. Improving your communication skills will benefit you greatly in both personal and professional settings.

You've openly admitted that you don't have the technical understanding to provide meaningful insights or solutions, yet you're still passing judgment. This is counterproductive. If you had deeper technical insights, you'd recognise that this issue is far from 'simple' due to the architectural complexities of the application and the maintainers' vision for the project. You've said you want to leave this to the experts- if that's truly the case, perhaps it's time to follow your own advice and reserve further comment.

While I acknowledge you're having issues, the majority of users, myself included, are not experiencing what you're describing. There may be specific factors in your setup contributing to the problem. For example, during my own debugging process, I identified issues with Traefik and a child networking component of this repo, where TLS overhead on mobile devices caused heat and slowdowns. Connecting directly over IP resolved the issue.

Effective debugging requires a reproducible scenario, and we don't have one yet. You've made no effort to provide useful data for troubleshooting, and instead, your commentary belittles those who are volunteering their time to help. That's completely inappropriate.

At the end of the day, we're all here to collaborate and improve things. A little mutual respect and openness to understanding will go a long way in achieving that.

This is the last time I'll engage with you on this, as we're deviating from the topic at hand. Unless you have something meaningfully constructive to contribute, I'd suggest you leave this to those, by your own admission, more skilled.

@jadenquinn
Copy link
Author

jadenquinn commented Sep 21, 2024

I am connecting over an internal network. I think I mentioned this before. No funny business between my phone and the machine running the Immich server, so there shouldn't be any overhead caused by that.

"leaving it to others" doesn't just mean they should do everything. I gave pointers, like "Hey! There! This is where the problem is, it's in there somewhere, you can fix it now!" That's what I'm trying to do. I've done this and am actively doing this with other devs, and it seems to work.
It's true, community efforts make up open source projects, it's people volunteering their time. That's great, but it's also problematic at times..

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

No branches or pull requests

5 participants