fix geotagging location selection, providers, lifecycle and staleness#662
Open
thomasbuilds wants to merge 6 commits into
Open
fix geotagging location selection, providers, lifecycle and staleness#662thomasbuilds wants to merge 6 commits into
thomasbuilds wants to merge 6 commits into
Conversation
getOptimalLocation() had several defects: - The accuracy tiebreaker was inverted. Location.getAccuracy() is a radius in meters where smaller is better, but the code preferred the larger value, so a less accurate fix replaced a more accurate one (inherited from the former getAccurateOne(), which always picked the least accurate location). - The staleness check was one-sided: only a significantly newer candidate was preferred on time. A significantly older candidate fell through to the accuracy comparison, so a stale fix could beat a fresh one depending on its position in the list, making the result order-dependent (the pairwise comparison isn't transitive around the staleness threshold). - Ages were compared via Location.getTime(), which is wall-clock time that can jump and can disagree between providers, instead of the monotonic elapsedRealtimeNanos. Replace the pairwise fold with a direct selection: take the newest location, and among the locations not significantly older than it, pick the most accurate one. This can't select a location that's significantly older than the freshest available one, is independent of candidate order, treats locations without accuracy information (getAccuracy() defaults to 0, which would always win a corrected comparison) as least accurate, and resolves accuracy ties to the newer location.
The batched onLocationChanged() overload replaced the stored location with the best fix from the batch unconditionally, unlike the single-location overload which compares against the stored fix. Merge the stored location into the comparison so a better existing fix isn't discarded.
requestLocationUpdates() only queried the last known locations when no location was stored, so a stale fix held in memory from an earlier camera session prevented picking up a fresher last known location. Query them unconditionally and let the selection logic decide.
Location updates were requested from every provider (gps, network, fused and passive at once), duplicating the streams the fused provider already combines and keeping the GNSS engine active whenever the camera is open. The fused-provider path added for the last known location query was also gated on being a system app, but the provider isn't restricted to system apps; whether the OS provides it is what matters, which hasProvider() answers directly. Use the fused provider alone for both the last known location query and live updates when the OS has it, falling back to gps and network (not passive) on older releases. This addresses the remaining part of GrapheneOS#530.
Updates were requested in onResume() but only removed when the user turned the setting off, so the listener stayed registered for the lifetime of the process after the activity went away. Drop the updates in onPause(); the stored location is kept so a quick app switch doesn't lose the current fix. When the user disables geotagging the stored location is now also cleared, since the app no longer needs it.
getLocation() returned whatever fix was stored with no bound on its age, so a last known location from hours or days earlier could be silently written into the EXIF data of a photo taken now. Return null when the stored fix is older than 15 minutes; the capturers already show the existing "location unavailable" message in that case. The 15 minute bound is chosen to stay above the OS throttling for apps holding only the approximate location permission, which receive at most one location every 10 minutes. Updates were also requested with a 10 m minimum distance, so a stationary user stopped receiving fixes and the stored one aged indefinitely even though it was still valid. Request updates with no distance filter and let the time interval do the throttling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #637 and the remaining part of #530. The one-line fix in #637 was incomplete on its own: with the comparison direction corrected, the one-sided staleness check still lets a significantly older but accurate fix win whenever it appears after a fresher one in a candidate list (last known location scans, batched updates), and the wall-clock time comparison it relies on isn't reliable across providers in the first place. This series fixes the selection logic as a whole plus the issues around it, one logical change per commit.
rewrite location selection to fix accuracy and staleness comparison: the accuracy tiebreaker was inverted (smaller
getAccuracy()is better), the staleness check was one-sided (a significantly older candidate fell through to the accuracy comparison, making the result order-dependent), and ages were compared via wall-clockLocation.getTime(), which the platform documentation says "should not be used to order or compare locations". The selection is now: take the newest fix (byelapsedRealtimeNanos), and among fixes not significantly older than it, pick the most accurate; missing accuracy ranks last, ties go to the newer fix. Property-tested (200k randomized candidate sets plus an exhaustive small-case sweep across all permutations): order-independent, never selects a fix significantly older than the freshest available.merge batched location updates with the stored location: the batched
onLocationChangedoverload replaced the stored fix without comparing against it, unlike the single-location overload.always merge last known locations instead of skipping when one is stored: a stale in-memory fix no longer prevents picking up a fresher last known location on re-attach.
use the fused location provider alone when available: previously updates were requested from every provider at once (gps, network, fused, passive), duplicating what fused already combines and forcing the GNSS engine on. The system-app gate from Consider both staleness and accuracy to decide optimal location #531 is replaced with
hasProvider(FUSED_PROVIDER): availability is the OS's contract, not the caller's privilege. Falls back to gps and network before S, or on the off chance fused is absent on a newer release.stop location updates while the activity is paused: the listener used to stay registered for the process lifetime; now it follows onResume/onPause, and disabling geotagging clears the stored fix.
avoid attaching stale locations to captures:
getLocation()returns null for fixes older than 15 minutes instead of silently writing an arbitrarily old position into EXIF; the existing "location unavailable" message covers that case.minDistancegoes from 10 m to 0 so stationary users keep receiving fixes (otherwise a still-valid fix ages past any bound).Points I'd like your input on:
The 15 minute bound. It must stay above the 10 minute delivery throttle the OS applies to apps holding only the approximate location permission (
LocationProviderManager.MIN_COARSE_INTERVAL_MS), otherwise approximate-permission users would get untagged captures for most of the interval between throttled deliveries. A stricter bound scaled by the granted permission (e.g. ~2 min with fine, 15 min with coarse) is possible if you prefer; kept simple for now.Fused availability assumption.
hasProvider(FUSED_PROVIDER)is trusted to mean the provider actually delivers. That holds for GrapheneOS and for stock S+ (platform fused provider / Play services); a ROM that registers an inert fused provider would break geotagging, but that would be an OS bug.Streaming window. Live updates are merged one at a time against the single stored fix, so during a provider transition (e.g. GNSS dying indoors) a more accurate fix seen up to 11 s before the current one may already have been discarded, and the selection can settle on a slightly worse fix than a full history would allow. Fixing that would mean keeping a window of recent fixes, which didn't seem worth the complexity.
Verified on a Pixel 9 end to end with a mock fused provider and per-photo EXIF checks: correct tag attached, accuracy tiebreak, newer-wins beyond the threshold, listener registration following the activity lifecycle (dumpsys), the 15 minute gate refusing an aged fix on both warm and cold start (with the snackbar shown), and video metadata (ISO 6709 atom). The same tiebreak scenario on main tagged the photo with a stale unrelated fix. Each commit compiles standalone.