883195 - remove MarkDirty

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

We should not allow SourceSurfaceData's data to be modified since that requires a fairly complex re-upload logic. Having it immutable also simplifies texture caching. Plus, this is unused. So lets nuke it. If you agree please be so kind and land this for me.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

May have been a place holder, the function/comment is it's more than a year and a half old, and unused, but it'd be good for Bas to confirm.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 762733 [details] [diff] [review]
patch

Review of attachment 762733 [details] [diff] [review]:
-----------------------------------------------------------------

We explicitly decided this to be a possibility. This was extensively discussed between me and Roc. There's a bunch of cases in which this is very desirable for example when decoding images where image data is added as decoded data comes in, and other such reasons. It's fundamentally desirable when making partial changes to surfaces to avoid having a lot of memory bandwidth used for large source surfaces.

If an efficient, optimized, cached texture is required DrawTarget::CreateSourceSurfaceForData will create an immutable source surface where the texture can be efficiently cached.

Comment 4

[Andreas Gal :gal]

I am not sure I agree. Can you help me understand why this API is more efficient than a partial upload of such newly arriving data?

With this current API you have to download the entire texture and then re-upload the entire texture (only architectures without shared VRAM) since you don't supply a dirty rect to MarkDirty.

If you really really want this, then we would at least need that dirty rect so we can do a partial upload. Thats still a clumsy interface though because we have to keep the texture data around twice, once in system memory in case you do GetDataSourceSurface so we don't have to download the whole texture all the time, and in the texture. If you don't keep it around twice, you download every time you get more image data. Thats attrociously slow.

Every which way I am looking at this, MarkDirty() doesn't make any sense. Can you enlighten me?

I would propose an explicit update API for what you are describing here, and in any case, we should not have dummy APIs no backend is implementing for a year.

How about SourceSurface->UpdateRect(pixel data) instead?

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Andreas Gal :gal from comment #4)
> I am not sure I agree. Can you help me understand why this API is more
> efficient than a partial upload of such newly arriving data?

DataSourceSurface is not -meant- to hold a link to an uploaded texture. A DataSourceSurface is explicitly to represent image data in RAM. For a texture you'd use something like SourceSurfaceD2D or SourceSurfaceSkiaTexture (for example). Which would be created through DrawTarget::CreateSourceSurfaceForData or DrawTarget::OptimizeSourceSurface. Those just implement SourceSurface and not DataSourceSurface so they are immutable. Only DataSourceSurface implementors are mutable, and a general SourceSurface is -not- meant to implement DataSourceSurface (unless it can cheaply do so, i.e. a software backend, in which case a partial update is cheap anyway!)

> With this current API you have to download the entire texture and then
> re-upload the entire texture (only architectures without shared VRAM) since
> you don't supply a dirty rect to MarkDirty.
> 
> If you really really want this, then we would at least need that dirty rect
> so we can do a partial upload. Thats still a clumsy interface though because
> we have to keep the texture data around twice, once in system memory in case
> you do GetDataSourceSurface so we don't have to download the whole texture
> all the time, and in the texture. If you don't keep it around twice, you
> download every time you get more image data. Thats attrociously slow.

GetDataSourceSurface is meant to be slow, we should -not- keep the data around in system memory 'in case' someone calls it. In the case of D2D for example, GetDataSourceSurface will trigger a readback.

> Every which way I am looking at this, MarkDirty() doesn't make any sense.
> Can you enlighten me?

Hopefully I've been clearer this time, I'm getting the impression I haven't quite made clear (and obviously the documentation also hasn't) the intended use pattern of source surfaces.

Comment 6

[Andreas Gal :gal]

Yeah, the documentation is a bit thin, which is ok. I am trying to figure out your design intend here. I can understand other parts by looking at the D2D backend, but this piece was unused.

If I understand you correct, DataSourceSurface should never try to hold a backing surface. We always upload it when its used against a DT again. In that case I have no objects to MarkDirty(). I might submit a patch with documentation for some of these pieces aa ask you for review.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think having SourceSurfaces be immutable except for DataSourceSurface is a bit unfortunate. Usually you want all the invariants of a superclass also apply to a subclass. So we have to say nothing about the mutability of SourceSurfaces and then document in each subclass whether it's mutable or not. That's ugly, but nothing obviously better springs to mind.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> I think having SourceSurfaces be immutable except for DataSourceSurface is a
> bit unfortunate. Usually you want all the invariants of a superclass also
> apply to a subclass. So we have to say nothing about the mutability of
> SourceSurfaces and then document in each subclass whether it's mutable or
> not. That's ugly, but nothing obviously better springs to mind.

Wait, how would you mutate anything -but- a DataSourceSurface, since they're opaque carriers of bits? You mean by directly accessing the underlying texture for example? Then you'd need a method to lock that texture anyway with the semantics of that specific SourceSurface, which means that can be done outside of the generic SourceSurface class and it would still be immutable unless the specific subclass has some way to mutate it.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not suggesting anything other than DataSourceSurface should be made mutable. I'm just saying it's ugly to have everything be immutable except for one specific subclass.

Comment 10

[Andreas Gal :gal]

Whats exactly the use-case here? I am streaming in pixel data and want to continuously upload that?

If I understand you correctly, you propose that we do:

DrawSurface(SourceSurface-with-first-bits)
and then to update that
DataSourceSurface = SourceSurface-with-first-bits->GetData (readback?)
while-more-data:
  update DataSourceSurface->Data
  DataSourceSurface->MarkDirty
  DrawSurface(DataSourceSurface)

This totally doesn't make sense to me still. Instead I would just do:

void *buffer-for-image-data
DrawSurface(CreateSourceSurfaceFromData(buffer-for-image-data))
while-more-data:
  update-buffer-for-image-data
  DrawSurface(CreateSourceSurfaceFromData(buffer-for-image-data))

I am still a bit fuzzy on when MarkDirty() is really useful.