Multiframe scrubbing improvements#397
Conversation
Deploying geodatalytics with
|
| Latest commit: |
3cbb2a0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://550b87fe.geodatalytics.pages.dev |
| Branch Preview URL: | https://multiframe-scrubbing-improve.geodatalytics.pages.dev |
bb34a8b to
aa64e06
Compare
|
be able to extract the opacity and default frame when calculating the style fingerprint. |
|
add in sorting to the dictionary for the styles to resolve any ordering of hashes. |
annehaley
left a comment
annehaley
left a comment
There was a problem hiding this comment.
Since this is a large PR, I'll do my reviews in two rounds. This first round is just from reading the diff. After these comments are addressed I'll do a second round where I try it out locally and can comment on the behavior.
| def _hex_to_rgb(color: str) -> tuple[int, int, int]: | ||
| color = color.lstrip("#") | ||
| return int(color[0:2], 16), int(color[2:4], 16), int(color[4:6], 16) | ||
|
|
||
|
|
||
| def _rgb_to_hex(rgb: tuple[int, int, int]) -> str: | ||
| return "#{:02X}{:02X}{:02X}".format(*rgb) |
There was a problem hiding this comment.
I see that this file is copying logic that previously only existed client-side. It makes sense that we need it on the server now too. But can we avoid having the same logic duplicated in two places (with two different languages)? That makes it harder to maintain. Maybe we could add an API endpoint on the style viewset that can provide the query string for a given style and use that result on the client side?
There was a problem hiding this comment.
What we have now:
- The client side has a query builder for large-image rasters so it can take colormaps/filters and create a preview while editing
- The internal storage of these colormaps and filters is stored in a separate data structure which is passed to the server when you save a style
- This JSON style specification is broken into pieces and saved as ColormapConfig, SizeConfig, FilterConfig and others
So when we request a style from the server for the client it needs to take the style JSON and build back up the query params for raster large-images.
Should during saving should we also save the large-image query params that the client has already created. The previews then can use that and break it up into it's params for large-image instead of trying to re-interpret the ColormapConfig and FilterConfig and generate the query params?
There was a problem hiding this comment.
Yeah, that idea is cleaner. We can add a tile_query field to the LayerStyle model (nullable, since vector styles won't need it). Then we don't need any new logic on the server side to convert the db representation of the style into a query.
| # Set while running in a context (e.g. `manage.py ingest`) where TaskResult | ||
| # WebSocket notifications are meaningless: tasks run synchronously with no | ||
| # client session listening. When set, result_post_save skips the push entirely | ||
| # rather than attempting it and logging a warning. | ||
| _suppress_notifications: ContextVar[bool] = ContextVar("suppress_task_notifications", default=False) |
There was a problem hiding this comment.
This is an interesting addition that I had never considered. I do think it's useful, but I think we should be applying this context differently. Right now, I see that there's one place where you've applied this context, in ingest.py:
with suppress_task_notifications():
invalidate_and_enqueue_previews(style, asynchronous=False)
If we do end up moving the preview generation to the dataset conversion task (see earlier comment), we can't always apply this context. But we can apply it if asynchronous=False. My suggestion is that we add the with suppress_task_notifications(): line inside invalidate_and_enqueue_previews right before we call .apply().
In fact, we can do that in all three places where we run a task synchronously. If you search the code for .apply(, you can add the context wrapper around all three places, which includes the convert_chart and convert_dataset tasks. This should make the suppression behavior more consistent for all synchronous tasks.
There was a problem hiding this comment.
Added the suppression to other locations that use .apply and have a sync and async path. I'll be moving the invalidate_and_enqueue_previews out of the ingest.py and into the dataset conversion process in a future PR.
This is the pr that has the base changes: c4525bd
| for layer in Layer.objects.filter(dataset=dataset, default_style__isnull=True): | ||
| layer.ensure_default_style() |
There was a problem hiding this comment.
I'm concerned about having a db-write side effect within a get endpoint. Is there anywhere else we could put this statement to ensure that layers have a default style? Same goes for the ensure_default_style call in uvdat/core/rest/layer.py.
Your comment about ensure_default_style explains that a layer can be left without a default style if its existing default style is deleted, so maybe we can just put that function call in a delete signal?
There was a problem hiding this comment.
This is an oversight on my part. I need to do a bit more thinking and it relates to the other questiona bout having a default_style LayerStyle object for use with the previews.
I haven't made a decision but I'm weighing the options
- Move the creation of the frame-previews and the default style for multiframe raster into the dataset ingestion task script
- additional write a migration that will create a layerstyle for any multiframe rasters that don't have one already
- Accept the case of not having a default_style and having the logic to handle this and properly generate one if none exists (probably again in a migration).
| if style is None: | ||
| style = LayerStyle.objects.create(name="Default", layer=layer, project=project) | ||
| style.save_style_configs(DEFAULT_MULTIFRAME_RASTER_STYLE_SPEC) |
There was a problem hiding this comment.
I'm not sure I fully understand why we need to create default style objects. The current behavior allows for the absence of any created styles on a layer; in that case, raster layers will just use the large-image defaults. Is there a reason we can no longer use that pattern? If it's only for the foreign key relationship on the preview, we could just allow that field to be null.
There was a problem hiding this comment.
I think it keeps some of the logic simpler if we have an explicit default layer style after data is ingested.
The preview system is essentially keyed on the LayerStyle. We can change it but we would need to add logic branches to handle fingerprinting, task invalidation, the preview_status in the HTTP response. I.E. conditionals of if layer.default_style is None: then do a slightly different thing using DEFAULT_MULTIFRAME_RASTER_STYLE_SPEC
There was a problem hiding this comment.
With your current approach, we need to ensure that every layer has a default style created. We only need them to exist for raster layers, but for the sake of consistency we make them for every layer. You've added this step in the ingest process, and you also ensure that step is completed for already-existing layers. You currently do this in the endpoint where we get all layers for a target dataset, and I've noted in my previous comment that I don't think we should do that in a get request. If you can find another place to do that, I think your approach is acceptable, but design-wise I still think a None style makes more sense.
How much more complex would it be to support a None style in those places you mentioned, compared to creating these default style objects? They seem equally complex to me.
There was a problem hiding this comment.
Yeah I completely agree with moving the creation out of the GET endpoint comments (that was an oversight on my part).
I think it would require that within the layer need some default Query URL (like in the other comment) for default_style and possible a fingerprint style spec for checking to make sure we really need to create new empty previews on save.
- Then in the code it would need changes, mostly more arguments because I access the layer a lot through the layer_style.layer:
invalidate_and_enqueue_previews- the layer_style passed in could be null meaning that the fingerprint would be some default fingerprint. So we need to pass in the Layer as well to this function because before we would access the layer by using the connected layer_style.layer- When
mark_previews_regeneratingit would have the new layer and the layer_style (which could be null) and create RasterFramePreview objects where the layer_style foreign key is null. clear_style_preview_instance_cachewould also need the Layer and the layer_style passed in the function to remove the previous cached prefetched resultssupersede_pending_preview_taskswould need the layer id in addition to the layer_style_id and the inputs for the task for the TaskResult would require the layer_id as well as the layer_style_id to be added to invalidate any previous tasks creating a style for the default style- when passing into
generate_layer_style_previews(the task for creating the previews) the layer_id would have to be added along with the layer_style id.
- Inside of the preview generation task:
- the
_PreviewGenerationContextwould now need the layer_id as well as the layer_style_id - in the creation of a frame preview
_process_frame_previewit would now use a git based on the layer_id, the layer_style (null being valid) and the frame for replacing/creating a new Preview object
- the
- Access for REST Responses:
preview_status_for_style(layer_style)- this was used before to determine if previews were ready for a style. I may need a whole seprate function ofpreview_status_for_empty_style(layer_id)that would check the RasterFramePreviews that match the layer_id and see if they are all ready for preview. In the original function layer_styles now had a foreign key to layer_style so I could uselayer_style.frame_previewsand see if they were all ready. Now I think I need to update this function to use the style_previews (foreign key into layerFrame). So this seprate function would take a layer -> get the layer_frames for it and then filter RasterFramePreviews for any matching LayerFrame Ids that have a null layer_style set.- The Layer serializer itself would return None if the obj.defaul_style_id is None. So instead we need to get the layer_frames from the layer, then filter to see what RasterFramePreviews have a null layer style and match the layer_frame for the null case of default style and return them as the previews. Again we can't utilize the foreign key of
layer_style.frame_previews - There may be some abstraction that can be done in the two above functions to create a single way to take a layer with no default style and return the frame previews to check their status or return their presigned URLS for serialization I just thought about it after walking through the logic.
- One thing I didn't consider is that I assume the empty default_layer_style for the raster frame previews just sticks around and is never deleted until the layer itself is deleted. That way if the user ever deletes the styles it would have this set of styles to fall back on.
That's a rough 15-20 minute analysis of what may need to change with a None default_style. Most of it has to do relying on the foreign key relationships between LayerStyle and RasterFramePreview (layer_style.frame_previews) being broken as well as utillizing layer_style.layer for reverse lookup of the layer.
| if result_id is None: | ||
| return None | ||
|
|
||
| result = TaskResult.objects.filter(id=result_id).first() |
There was a problem hiding this comment.
Just use TaskResult.objects.get(id=result_id)
There was a problem hiding this comment.
I'm using the filter there to prevent a DoesNotExist error that would happen with get where instead I want None. When the result is None it means that the task result has been superseded by another task. I.E while generating previews a user chained their mind so it started another task for generating previews.
I could update it to use try block with catching DoesNotExist. I could also add a comment as to why I'm using filter here.
There was a problem hiding this comment.
Ok that makes sense. A comment would be sufficient, then.
There was a problem hiding this comment.
added a descriptivie comment in: 515223e
| const layerStore = useLayerStore(); | ||
| const mapStore = useMapStore(); | ||
| const styleStore = useStyleStore(); |
There was a problem hiding this comment.
For these other stores that you use in this store's functions, just get them all at the top of the store definition. That's what we do in all the other stores.
2 participants
Resolves #126
Summary
Improves multiframe raster scrubbing by fixing RGB detection for sequential rasters, adding server-generated frame preview images, and crossfading from preview to tiles on the client for smoother frame transitions.
Done
Sentinel downloader (
scripts/sentinelDownload/)--single-file(usesgdal_translateto append frames as subdatasets)Dataset / RGB fixes
band:1injection removed when nosource_filteris supplied — this was preventing Boston ortho imagery from being recognized as RGBband:1is still applied when explicitly defined in the style specframeandbandare extracted from the style JSON and passed as separate query parameters (outsidestyle) so django-large-image can auto-detect RGB imageryRasterFramePreviewmodelLayerStyleandLayerFrame(unique per style/frame pair)width,height, georeferencedbounds, and an S3 preview imagestatus:creating|regenerating|complete|failedstyle_fingerprint(sha256ofrepr_style_configs()) guards against stale concurrent generationpost_deletesignaluvdat/core/frame_previews/raster_style.py- Builds django-large-image–compatible style objects fromLayerStyleDB records; keepsframe/bandoutsidestylepreview_regeneration.py- Marks previews stale, supersedes pending tasks, enqueues Celery regeneration on style save/ingestfingerprint.py- Computes style fingerprint for concurrency controltypes.py- SharedFramePreviewDatatypeuvdat/core/tasks/frame_preview.pyraster_style.pyfor styling; scales resolution to keep previews between ~1k–4kRasterFramePreviewrows with bounds and dimensionsTaskResulton completion (used for future client notification)Preview regeneration on style save
LayerStylecreate/update hooks callinvalidate_and_enqueue_previews()regeneratingstatus=completepreviews with an image are serialized to the APIAPI
/layers/and/layer-styles/returnmultiframe_previews— ordered list of{ bounds, width, height, url }per raster frame (presigned S3 URLs)preview_statusaggregated on layer and layer-style:ready|generating|notready(omitted when N/A)Tests
uvdat/core/tests/test_frame_preview.py— model serialization, API fields, regeneration hooks, fingerprint guards, raster style helpersFrame Preview Logic
Loading Frame Preview Flow
layerStore.updateLayersShownstyleStore.updateLayerStylesframePreviewStore.showPreviewThenTilesraster-opacity: 0)web/src/utils/framePreviewLayer.tsFunctionsupsertPreviewLayer— fetch preview URL, add/update MapLibre source + raster layerhidePreviewLayer— set visibility tononeduring transitionremovePreviewLayer/removePreviewLayersExcept— cleanup; keep adjacent preloaded frameswaitForRasterSourceLoaded— poll until tile source is ready (with timeout)fadeRasterOpacities— animated crossfade between preview and tile layersweb/src/store/framePreview.tsFunctionsprefetchLayerPreviews— preload all preview images for a layershowPreviewThenTiles— main preview → tile transition orchestrationdismissPreviewForLayer— skip previews while editing layer styles; restore tile opacityStyle editing integration
styleStore.setLayerStyleEditingdismisses previews when entering style edit modeupdateLayerStylesafter exiting edit modeLayerStyle.vuewatches active layer to toggle editing stateCreating/Editing and Saving Style Flow
LayerStyle.vueLayerStyleSerializer.create/.update)invalidate_and_enqueue_previewsfor multifram rasters onlystyle_fingerprint- hash for comparing to see if the style changedpreviews_current_for_fingerprint- if the fingerprint is the same (same style) don't invalidate and generate new previews, instead just return.preview_statusofnot readyto indicate that the previews are being created.markStyleSavedAndInvalidatePreviews(style)- clear the existing previews for the stylestyleStore.selectedLayerStyleswithmultiframe_previews: undefinedframePreviewStore.dismissPreviewForLayerrefreshLayer() -> layerStore.fetchAvailableLayer (refreshes layer metadata if default changed)
framePreviewthat lets the client know that new previews are availableframePreviewStore.onPreviewTaskComplete(task)getLayerStyle(layer_style_id)- refetch style withpreview_status: "ready"+ multiframe_previewsprefetchLayerPreviews- warm image URLsshowPreviewThenTiles-> preview overlay + crossfade back to tiles