-
Notifications
You must be signed in to change notification settings - Fork 246
feat(photo-processing): implement location processing and geocoding system #95
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ builder.config.json | |
| apps/web/assets-git | ||
| apps/web/public/thumbnails | ||
| apps/web/src/data/photos-manifest.json | ||
| apps/web/src/data/.geocode-cache.json | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .geocode-cache.json should upload to remote repo cache |
||
| .vercel | ||
| photos | ||
| */*/.next | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ export const ExifPanel: FC<{ | |
|
|
||
| onClose?: () => void | ||
| }> = ({ currentPhoto, exifData, onClose }) => { | ||
| const { t } = useTranslation() | ||
| const { t, i18n } = useTranslation() | ||
| const isMobile = useMobile() | ||
| const formattedExifData = formatExifData(exifData) | ||
| const isExiftoolLoaded = useAtomValue(isExiftoolLoadedAtom) | ||
|
|
@@ -45,6 +45,36 @@ export const ExifPanel: FC<{ | |
| const decimalLatitude = gpsData?.latitude || null | ||
| const decimalLongitude = gpsData?.longitude || null | ||
|
|
||
| const locationDisplay = useMemo(() => { | ||
| const loc = currentPhoto.location | ||
| if (!loc) return null | ||
|
|
||
| // Allow showing detailed location directly from Nominatim's display_name | ||
| const showDetailed = | ||
| (import.meta as any).env?.VITE_SHOW_DETAILED_LOCATION === 'true' | ||
| if (showDetailed && loc.displayName) return loc.displayName | ||
|
|
||
| const city = loc.city || null | ||
| const province = loc.province || null | ||
| const country = loc.country || null | ||
|
|
||
| // Order rule: default "city, province, country"; for zh/ja languages use "country province city" | ||
| const lang = (navigator?.language || 'en').toLowerCase() | ||
| const activeLang = (i18n?.language || lang).toLowerCase() | ||
| const isEastAsiaOrder = | ||
| activeLang.startsWith('zh') || | ||
| activeLang.startsWith('ja') || | ||
| activeLang === 'jp' | ||
|
Comment on lines
+64
to
+67
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about ko?
Comment on lines
+64
to
+67
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ja or jp? |
||
|
|
||
| const parts = isEastAsiaOrder | ||
| ? [country, province, city].filter(Boolean) | ||
| : [city, province, country].filter(Boolean) | ||
|
|
||
| if (parts.length === 0) return null | ||
| // For East Asian order, use spaces; otherwise commas | ||
| return isEastAsiaOrder ? parts.join(' ') : parts.join(', ') | ||
| }, [currentPhoto]) | ||
|
Comment on lines
+75
to
+76
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc in China we don't use space to separate address. like |
||
|
|
||
| // 使用通用的图片格式提取函数 | ||
| const imageFormat = getImageFormat( | ||
| currentPhoto.originalUrl || currentPhoto.s3Key || '', | ||
|
|
@@ -578,6 +608,9 @@ export const ExifPanel: FC<{ | |
| value={`${formattedExifData.gps.altitude}m`} | ||
| /> | ||
| )} | ||
| {locationDisplay && ( | ||
| <Row label={'Location'} value={locationDisplay} /> | ||
| )} | ||
|
|
||
| {/* Maplibre MiniMap */} | ||
| {decimalLatitude !== null && decimalLongitude !== null && ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import type { PickedExif } from '../../types/photo.js' | ||
|
|
||
| export function convertExifGPSToDecimal( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i remember there is already a function for this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use |
||
| exif: PickedExif | null, | ||
| ): { latitude: number; longitude: number } | null { | ||
| if (!exif) return null | ||
| let latitude: number | null = null | ||
| let longitude: number | null = null | ||
|
|
||
| if (typeof exif.GPSLatitude === 'number') { | ||
| latitude = exif.GPSLatitude | ||
| } else if (exif.GPSLatitude) { | ||
| const num = Number(exif.GPSLatitude) | ||
| latitude = Number.isFinite(num) ? num : null | ||
| } | ||
|
|
||
| if (typeof exif.GPSLongitude === 'number') { | ||
| longitude = exif.GPSLongitude | ||
| } else if (exif.GPSLongitude) { | ||
| const num = Number(exif.GPSLongitude) | ||
| longitude = Number.isFinite(num) ? num : null | ||
| } | ||
|
|
||
| if (latitude === null || longitude === null) return null | ||
|
|
||
| const latSouth = | ||
| exif.GPSLatitudeRef === 'S' || exif.GPSLatitudeRef === 'South' | ||
| const lonWest = | ||
| exif.GPSLongitudeRef === 'W' || exif.GPSLongitudeRef === 'West' | ||
|
|
||
| const lat = latSouth ? -Math.abs(latitude) : Math.abs(latitude) | ||
| const lon = lonWest ? -Math.abs(longitude) : Math.abs(longitude) | ||
|
|
||
| if (!Number.isFinite(lat) || !Number.isFinite(lon)) return null | ||
| return { latitude: lat, longitude: lon } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| import fs from 'node:fs/promises' | ||
| import path from 'node:path' | ||
|
|
||
| import { workdir } from '../../path.js' | ||
|
|
||
| export type ReverseGeocodeResult = { | ||
| city: string | null | ||
| province: string | null | ||
| country: string | null | ||
| displayName?: string | null | ||
| } | ||
|
|
||
| type NominatimResponse = { | ||
| display_name?: string | ||
| address?: Record<string, string> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider using a more accurate type hint for this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's okay to use manifest, scanning the corresponding fields in builder can detect whether cache data exists. |
||
| } | ||
|
|
||
| const CACHE_FILE = path.join(workdir, 'src/data/.geocode-cache.json') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO for reviewer: why do we need a new cache file. Any recommendation? Would it be possible to directly use manifest
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @Innei |
||
| const CACHE_TTL_MS = 365 * 24 * 60 * 60 * 1000 // 1 year | ||
|
|
||
| type CacheEntry = { v: ReverseGeocodeResult; t: number } | ||
| type CacheData = Record<string, CacheEntry> | ||
|
|
||
| let cache: CacheData | null = null | ||
|
|
||
| async function loadCache(): Promise<CacheData> { | ||
| if (cache) return cache | ||
| try { | ||
| const raw = await fs.readFile(CACHE_FILE, 'utf-8') | ||
| const json = JSON.parse(raw) as CacheData | ||
| cache = json || {} | ||
| } catch { | ||
| cache = {} | ||
| } | ||
| return cache! | ||
| } | ||
|
|
||
| async function saveCache(): Promise<void> { | ||
| if (!cache) return | ||
| try { | ||
| await fs.mkdir(path.dirname(CACHE_FILE), { recursive: true }) | ||
| await fs.writeFile(CACHE_FILE, JSON.stringify(cache), 'utf-8') | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } | ||
|
|
||
| const roundCoord = (n: number, decimals = 3) => Number(n.toFixed(decimals)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why 3? |
||
| const makeKey = (lat: number, lon: number, lang: string) => | ||
| `${roundCoord(lat)},${roundCoord(lon)}@${lang}` | ||
|
|
||
| let queue: Promise<unknown> = Promise.resolve() | ||
| let lastAt = 0 | ||
| const schedule = <T>(fn: () => Promise<T>): Promise<T> => { | ||
| const res = queue.then(async () => { | ||
| const now = Date.now() | ||
| const wait = Math.max(0, 1000 - (now - lastAt)) | ||
| if (wait > 0) { | ||
| await new Promise((r) => setTimeout(r, wait)) | ||
| } | ||
| lastAt = Date.now() | ||
| return fn() | ||
| }) as Promise<T> | ||
| queue = res.then( | ||
| () => {}, | ||
| () => {}, | ||
| ) | ||
| return res | ||
| } | ||
|
|
||
| const extractCityProvinceCountry = ( | ||
| data: NominatimResponse, | ||
| ): ReverseGeocodeResult => { | ||
| const a = data.address || {} | ||
| const city = | ||
| a.city || | ||
| a.town || | ||
| a.village || | ||
| a.municipality || | ||
| a.city_district || | ||
| a.suburb || | ||
| a.county || | ||
| a.hamlet || | ||
| null | ||
|
|
||
| const province = | ||
| a.province || | ||
| a.state || | ||
| a.region || | ||
| (a.state_district as string | undefined) || | ||
| null | ||
|
|
||
| const country = a.country || null | ||
|
|
||
| return { city: city || null, province, country } | ||
| } | ||
|
|
||
| const inflight = new Map<string, Promise<ReverseGeocodeResult>>() | ||
|
|
||
| export async function reverseGeocode( | ||
| lat: number, | ||
| lon: number, | ||
| ): Promise<ReverseGeocodeResult | null> { | ||
| if (!Number.isFinite(lat) || !Number.isFinite(lon)) return null | ||
| await loadCache() | ||
| const acceptLanguage = (process.env.NOMINATIM_ACCEPT_LANGUAGE || 'en').trim() | ||
| const key = makeKey(lat, lon, acceptLanguage) | ||
|
|
||
| // cache hit | ||
| const hit = cache![key] | ||
| if (hit && Date.now() - hit.t < CACHE_TTL_MS) { | ||
| return hit.v | ||
| } | ||
|
|
||
| const existing = inflight.get(key) | ||
| if (existing) return existing | ||
|
|
||
| const p = schedule(async () => { | ||
| const url = `https://nominatim.openstreetmap.org/reverse?lat=${encodeURIComponent( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://operations.osmfoundation.org/policies/nominatim/ nominatim requires a 1 qps limit for its api |
||
| lat, | ||
| )}&lon=${encodeURIComponent(lon)}&format=json&accept-language=${encodeURIComponent( | ||
| acceptLanguage, | ||
| )}` | ||
| try { | ||
| const res = await fetch(url, { | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'User-Agent': 'afilmory-builder/1.0', | ||
| }, | ||
| }) | ||
| if (!res.ok) throw new Error(`HTTP ${res.status}`) | ||
| const data = (await res.json()) as NominatimResponse | ||
| const result = { | ||
| ...extractCityProvinceCountry(data), | ||
| displayName: data.display_name || null, | ||
| } | ||
| cache![key] = { v: result, t: Date.now() } | ||
| await saveCache() | ||
| return result | ||
| } catch { | ||
| // store negative cache to avoid retry storms | ||
| const result: ReverseGeocodeResult = { | ||
| city: null, | ||
| province: null, | ||
| country: null, | ||
| displayName: null, | ||
| } | ||
| cache![key] = { v: result, t: Date.now() } | ||
| await saveCache() | ||
| return result | ||
| } finally { | ||
| inflight.delete(key) | ||
| } | ||
| }) | ||
|
|
||
| inflight.set(key, p) | ||
| return p | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ import type { PhotoManifestItem } from '../types/photo.js' | |
| import { shouldProcessPhoto } from './cache-manager.js' | ||
| import { | ||
| processExifData, | ||
| processLocation, | ||
| processThumbnailAndBlurhash, | ||
| processToneAnalysis, | ||
| } from './data-processors.js' | ||
|
|
@@ -228,6 +229,12 @@ export async function executePhotoProcessingPipeline( | |
| size: obj.Size || 0, | ||
| exif: exifData, | ||
| toneAnalysis, | ||
| location: await processLocation( | ||
| exifData, | ||
| photoKey, | ||
| existingItem, | ||
| options, | ||
| ), | ||
|
Comment on lines
+232
to
+237
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an option to disable reverse geoencoding. Due to the rate limit, it would be extremely slow to build manifest for users with many photos |
||
| // Live Photo 相关字段 | ||
| isLivePhoto: livePhotoResult.isLivePhoto, | ||
| livePhotoVideoUrl: livePhotoResult.livePhotoVideoUrl, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using config.json instead of env? https://github.com/Afilmory/afilmory/blob/main/config.example.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or builder.config.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove this since we already use config.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use builder.config.json instead