Closed Bug 792207 Opened 7 years ago Closed 7 years ago

[Azure] Add recording backend for Azure.

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(6 files, 2 obsolete files)

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.
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.
Attachment #662294 - Flags: review?(jmuizelaar)
This allows the Recording system to track scaled font lifetime.
Attachment #662320 - Flags: review?(jmuizelaar)
Attachment #662429 - Flags: review?(jmuizelaar)
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
Attachment #662312 - Flags: review?(jmuizelaar) → review+
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?
Attachment #662294 - Flags: review?(jmuizelaar) → review-
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.
Attachment #662294 - Attachment is obsolete: true
Attachment #662644 - Flags: review?(jmuizelaar)
Attachment #662644 - Attachment description: Retain ScaledFont objects for gfxFonts v2 → Part 1: Retain ScaledFont objects for gfxFonts v2
Attachment #662320 - Flags: review?(jmuizelaar) → review+
Attachment #662644 - Flags: review?(jmuizelaar) → review+
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.
Attachment #662429 - Flags: review?(jmuizelaar) → review-
Attachment #662429 - Attachment is obsolete: true
Attachment #663458 - Flags: review?(jmuizelaar)
Attachment #663460 - Flags: review?(jmuizelaar) → review+
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.
Attachment #663458 - Flags: review?(jmuizelaar) → review+
Attachment #663459 - Flags: review?(jmuizelaar) → review+
(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?
(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.
Depends on: 795655
No longer depends on: 1454027
You need to log in before you can comment on or make changes to this bug.