792207 - [Azure] Add recording backend for Azure.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

I've been working on a new record/replay system for Azure. This bug will contain the patches needed to make recording work on mozilla-central builds.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Retain ScaledFont objects for gfxFonts

Right now we recreate the ScaledFont each time we create a gfxFont, this is wasteful and causes recording to record truetype data every time a gfxFont is used. This patch fixes that.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Allow storing and creating from TrueType data

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add UserData to ScaledFont objects

This allows the Recording system to track scaled font lifetime.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Add DrawTargetRecording and DrawEventRecorder code.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 662312 [details] [diff] [review]
Part 2: Allow storing and creating from TrueType data

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

::: gfx/2d/ScaledFontDWrite.cpp
@@ +18,5 @@
> +    uint8_t *mData;
> +    uint32_t mSize;
> +};
> +
> +class DWriteFontFileLoader : public IDWriteFontFileLoader

What do we need to do to use this code to replace gfxDWriteFontFileLoader?

@@ +76,5 @@
> +  }
> +
> +private:
> +  static IDWriteFontFileLoader* mInstance;
> +}; 

trailing whitespace

@@ +136,5 @@
> +  virtual HRESULT STDMETHODCALLTYPE GetLastWriteTime(OUT UINT64* lastWriteTime);
> +
> +private:
> +  std::vector<uint8_t> mData;
> +  uint32_t mRefCnt;

the type should probably be ULONG

@@ +137,5 @@
> +
> +private:
> +  std::vector<uint8_t> mData;
> +  uint32_t mRefCnt;
> +}; 

trailing whitespace

@@ +192,5 @@
> +                                       UINT64 fragmentSize,
> +                                       void **fragmentContext)
> +{
> +  // We are required to do bounds checking.
> +  if (fileOffset + fragmentSize > (UINT64)mData.size()) {

c++ cast

@@ +197,5 @@
> +    return E_FAIL;
> +  }
> +  // We should be alive for the duration of this.
> +  *fragmentStart = &mData[fileOffset];
> +  *fragmentContext = NULL;

nullptr

@@ +303,5 @@
> +  file->GetReferenceKey(&referenceKey, &refKeySize);
> +
> +  RefPtr<IDWriteFontFileLoader> loader;
> +  file->GetLoader(byRef(loader));
> +  

trailing whitespace

@@ +315,5 @@
> +  void *context;
> +  stream->ReadFileFragment(&fragmentStart, 0, fileSize, &context);
> +
> +  aDataCallback((uint8_t*)fragmentStart, fileSize, mFontFace->GetIndex(), mSize, aBaton);
> +

c++cast

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 662294 [details] [diff] [review]
Part 1: Retain ScaledFont objects for gfxFonts

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

This should remove the caching that is already in gfxMacFont.cpp

::: gfx/thebes/gfxFont.cpp
@@ +1894,5 @@
>        // draw any remaining glyphs
>        glyphs.Flush(cr, aDrawMode, isRTL, aObjectPaint, globalMatrix, true);
>  
>      } else {
> +      RefPtr<ScaledFont> scaledFont = GetScaledFont(dt);

Does this break other platforms?

::: gfx/thebes/gfxFont.h
@@ +1695,5 @@
>      nsAutoPtr<gfxFontShaper>   mHarfBuzzShaper;
>  #ifdef MOZ_GRAPHITE
>      nsAutoPtr<gfxFontShaper>   mGraphiteShaper;
>  #endif
> +    mozilla::RefPtr<mozilla::gfx::ScaledFont> mMozScaledFont;

Why Moz?

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Retain ScaledFont objects for gfxFonts v2

There was indeed some other problems here. This should fix them although the superclass GetScaledFont implementation should perhaps do caching itself, but we should look at that on a per-platform basis. Perhaps the cairo font creation portion could also be cross-platform.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 662429 [details] [diff] [review]
Part 4: Add DrawTargetRecording and DrawEventRecorder code.

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

Missing DrawEventRecorder

::: gfx/2d/DrawTargetRecording.h
@@ +12,5 @@
> +#ifdef _MSC_VER
> +#include <hash_set>
> +#else
> +#include <unordered_set>
> +#endif

I don't see where you use these.

::: gfx/2d/Makefile.in
@@ +41,5 @@
>          PathCairo.cpp \
> +		DrawTargetRecording.cpp \
> +		PathRecording.cpp \
> +		RecordedEvent.cpp \
> +		DrawEventRecorder.cpp \

Whitespace is wrong here

::: gfx/2d/PathRecording.h
@@ +14,5 @@
> +
> +struct PathOp
> +{
> +  enum OpType {
> +    OP_MOVETO = 0,

How about OP_MOVE_TO etc.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Add DrawTargetRecording and DrawEventRecorder code. v2

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 5: Allow a global recorder to be used

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Part 6: Use a pref to select using recording

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 663458 [details] [diff] [review]
Part 4: Add DrawTargetRecording and DrawEventRecorder code. v2

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

::: gfx/2d/RecordedEvent.h
@@ +23,5 @@
> +// A change in minor revision means additions of new events. New streams will
> +// not play in older players.
> +const uint16_t kMinorRevision = 0;
> +
> +struct ReferencePtr

I'd call it SerializablePtr

@@ +150,5 @@
> +    STROKE,
> +    DRAWSURFACE,
> +    DRAWSURFACEWITHSHADOW,
> +    PATHCREATION,
> +    PATHDESTRUCTION,

Please separate with underscores.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e307925e4f29
https://hg.mozilla.org/mozilla-central/rev/30bde31be9d4
https://hg.mozilla.org/mozilla-central/rev/68876e7d17ea
https://hg.mozilla.org/mozilla-central/rev/ed2e2da7d098
https://hg.mozilla.org/mozilla-central/rev/ee2e60a30934
https://hg.mozilla.org/mozilla-central/rev/5ad72b579f87

Should this have tests of some sort?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ryan VanderMeulen from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/ed2e2da7d098

I don't guess this was meant to include a .rej file?

Comment 15

[Bas Schouten (:bas.schouten)]

(In reply to :Ms2ger from comment #14)
> (In reply to Ryan VanderMeulen from comment #13)
> > https://hg.mozilla.org/mozilla-central/rev/ed2e2da7d098
> 
> I don't guess this was meant to include a .rej file?

It was not, I must've screwed something up, I'll clean it up.