From e64cdae26ea1e4b07d23a22ba31933c444e5d32e Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 15:04:36 +0200 Subject: [PATCH 1/6] Preserve marker schemas when comparing profiles Combine schemas from all input profiles instead of keeping only the first profile's schemas. Schema-driven screenshots need their field definitions to translate image URL indexes and create screenshot tracks, including when only the second profile contains screenshots. --- src/profile-logic/merge-compare.ts | 15 ++++++-- src/test/unit/merge-compare.test.ts | 56 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/profile-logic/merge-compare.ts b/src/profile-logic/merge-compare.ts index a588e739e9..c13f550acd 100644 --- a/src/profile-logic/merge-compare.ts +++ b/src/profile-logic/merge-compare.ts @@ -124,9 +124,20 @@ export function mergeProfilesForDiffing( (resultProfile.meta as any)[key] = value; } } - // Ensure it has a copy of the marker schema and categories, even though these could + // Combine the marker schemas of all profiles, so that markers coming from + // any of them keep being understood. + const markerSchemaNames = new Set(); + resultProfile.meta.markerSchema = []; + for (const profile of profiles) { + for (const schema of profile.meta.markerSchema) { + if (!markerSchemaNames.has(schema.name)) { + markerSchemaNames.add(schema.name); + resultProfile.meta.markerSchema.push(schema); + } + } + } + // Ensure it has a copy of the categories, even though these could // be different between the two profiles. - resultProfile.meta.markerSchema = profiles[0].meta.markerSchema; resultProfile.meta.categories = profiles[0].meta.categories; resultProfile.meta.interval = Math.min( diff --git a/src/test/unit/merge-compare.test.ts b/src/test/unit/merge-compare.test.ts index 0c03235189..39e11e3abb 100644 --- a/src/test/unit/merge-compare.test.ts +++ b/src/test/unit/merge-compare.test.ts @@ -367,6 +367,62 @@ describe('mergeProfilesForDiffing function', function () { ]); }); + it('keeps the screenshot markers of a profile when the other profile has none', function () { + const { profile: profileA } = getProfileFromTextSamples('A B C'); + const { profile: profileB, stringTable: stringTableB } = + getProfileFromTextSamples('D'); + const screenshotUrl = 'Screenshot Url'; + addMarkersToThreadWithCorrespondingSamples( + profileB.threads[0], + profileB.shared, + [ + [ + 'CompositorScreenshot 0', + 1, + 2, + { + type: 'CompositorScreenshot', + url: stringTableB.indexForString(screenshotUrl), + windowID: '0', + windowWidth: 300, + windowHeight: 150, + }, + ], + ] + ); + profileB.meta.markerSchema.push({ + name: 'CompositorScreenshot', + display: ['marker-chart', 'marker-table'], + fields: [{ key: 'url', label: 'Image', format: 'unique-string' }], + }); + + const profileState = stateFromLocation({ + pathname: '/public/fakehash1/', + search: '?thread=0&v=3', + hash: '', + }); + const { profile: mergedProfile } = mergeProfilesForDiffing( + [profileA, profileB], + [profileState, profileState] + ); + + expect(mergedProfile.meta.markerSchema.map(({ name }) => name)).toContain( + 'CompositorScreenshot' + ); + + const mergedStringTable = StringTable.withBackingArray( + mergedProfile.shared.stringArray + ); + const screenshotData = ensureExists( + mergedProfile.threads[1].markers.data.find( + (data) => data !== null && data.type === 'CompositorScreenshot' + ) + ); + expect( + mergedStringTable.getString(ensureExists((screenshotData as any).url)) + ).toBe(screenshotUrl); + }); + it('should preserve transforms and produce matching call trees', () => { /** * This test verifies that when profiles are merged with transforms applied: From 69652798a0606aa76968480bb5acd3993d3450a1 Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 15:07:10 +0200 Subject: [PATCH 2/6] Add screenshot marker field formats Add schema formats for image string indexes and window dimensions so marker rendering can describe screenshots without knowing their payload field names. Resolve the image aspect ratio through a sibling size field and keep image data out of text formatting and marker searches. --- docs-developer/CHANGELOG-formats.md | 4 +++ src/components/tooltip/Marker.tsx | 33 ++++++++++++++++--- src/profile-logic/marker-schema.ts | 21 ++++++++++-- .../__snapshots__/marker-schema.test.ts.snap | 27 +++++++++++++++ src/test/unit/marker-schema.test.ts | 23 ++++++++++++- src/types/markers.ts | 9 ++++- 6 files changed, 109 insertions(+), 8 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index 146ea0a602..82a15e43f5 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -6,6 +6,10 @@ Note that this is not an exhaustive list. Processed profile format upgraders can ## Processed profile format +### Version 76 + +Two marker schema field formats were added: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. + ### Version 75 The func table (`profile.shared.funcTable`) representation changed, mirroring the v71 frame table change: diff --git a/src/components/tooltip/Marker.tsx b/src/components/tooltip/Marker.tsx index a7728ca21a..d99fbe51c0 100644 --- a/src/components/tooltip/Marker.tsx +++ b/src/components/tooltip/Marker.tsx @@ -56,6 +56,7 @@ import type { MarkerSchemaByName, MarkerIndex, MarkerFormatType, + MarkerPayload, InnerWindowID, Page, Pid, @@ -294,7 +295,8 @@ class MarkerTooltipContents extends React.PureComponent { value, thread.stringTable, threadIdToNameMap, - processIdToNameMap + processIdToNameMap, + data )} ); @@ -587,7 +589,7 @@ const URL_REGEXP = /^(https?:\/\/)\S+$/; /** * This function may return structured markup for some types suchs as table, - * list, or urls. For other types this falls back to formatFromMarkerSchema + * list, urls, or images. For other types this falls back to formatFromMarkerSchema * above. */ export function renderMarkerFieldValue( @@ -596,7 +598,9 @@ export function renderMarkerFieldValue( value: any, stringTable: StringTable, threadIdToNameMap?: Map, - processIdToNameMap?: Map + processIdToNameMap?: Map, + // The payload the value comes from, for formats that refer to a sibling field. + payload?: MarkerPayload | null ): React.ReactElement | string { if (value === undefined || value === null) { console.warn(`Formatting ${value} for ${JSON.stringify(markerType)}`); @@ -653,7 +657,8 @@ export function renderMarkerFieldValue( cell, stringTable, threadIdToNameMap, - processIdToNameMap + processIdToNameMap, + payload )} ); @@ -665,6 +670,26 @@ export function renderMarkerFieldValue( ); } + case 'screenshot-data-url': { + const size = (payload as any)?.[format.sizeFieldForAspectRatio]; + return ( + + ); + } default: throw new Error( `Unknown format type ${JSON.stringify(format as never)}` diff --git a/src/profile-logic/marker-schema.ts b/src/profile-logic/marker-schema.ts index 438f2611c6..10fd49bc7e 100644 --- a/src/profile-logic/marker-schema.ts +++ b/src/profile-logic/marker-schema.ts @@ -631,9 +631,12 @@ export function formatFromMarkerSchema( rows.push(...cellRows); return rows.map((row) => `(${row.join(', ')})`).join(','); } + case 'screenshot-data-url': + // Don't expand a base64-encoded image into text output. + return '(screenshot)'; default: throw new Error( - `Unknown format type ${JSON.stringify(format.type as never)}` + `Unknown format type ${JSON.stringify(format as never)}` ); } } @@ -686,6 +689,8 @@ export function formatFromMarkerSchema( formatFromMarkerSchema(markerType, 'string', v, stringTable) ) .join(', '); + case 'screenshot-size': + return `${value.width}px × ${value.height}px`; default: console.warn( `A marker schema of type "${markerType}" had an unknown format ${JSON.stringify( @@ -719,7 +724,16 @@ export function markerPayloadMatchesSearch( continue; } - if (isStringIndexFormat(payloadField.format)) { + const { format } = payloadField; + if (typeof format === 'object' && format.type === 'screenshot-data-url') { + // Searching a base64-encoded image isn't useful. + continue; + } + if (format === 'screenshot-size') { + value = formatFromMarkerSchema(data.type, format, value, stringTable); + } + + if (isStringIndexFormat(format)) { if (typeof value !== 'number') { console.warn( `In marker ${marker.name}, the key ${payloadField.key} has an invalid value "${value}" as a unique string, it isn't a number.` @@ -750,6 +764,9 @@ export function markerPayloadMatchesSearch( export function isStringIndexFormat( format: MarkerFormatType | undefined ): boolean { + if (typeof format === 'object') { + return format.type === 'screenshot-data-url'; + } return ( format === 'unique-string' || format === 'flow-id' || diff --git a/src/test/unit/__snapshots__/marker-schema.test.ts.snap b/src/test/unit/__snapshots__/marker-schema.test.ts.snap index 9f60bce48e..50216e9a4f 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.ts.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.ts.snap @@ -278,6 +278,33 @@ Array [ , "a, b", ], + Array [ + "screenshot-size", + Object { + "height": 1000, + "width": 1280, + }, + "1280px × 1000px", + "1280px × 1000px", + ], + Array [ + Object { + "sizeFieldForAspectRatio": "windowSize", + "type": "screenshot-data-url", + }, + 0, + , + "(screenshot)", + ], ] `; diff --git a/src/test/unit/marker-schema.test.ts b/src/test/unit/marker-schema.test.ts index 5b46aa02b6..dabe96ea04 100644 --- a/src/test/unit/marker-schema.test.ts +++ b/src/test/unit/marker-schema.test.ts @@ -8,6 +8,7 @@ import { parseLabel, extensionTextMarkerSchema, markerSchemaFrontEndOnly, + isStringIndexFormat, } from '../../profile-logic/marker-schema'; import { renderMarkerFieldValue } from 'firefox-profiler/components/tooltip/Marker'; import type { @@ -591,8 +592,17 @@ describe('marker schema formatting', function () { ], ['list', []], ['list', ['a', 'b']], + ['screenshot-size', { width: 1280, height: 1000 }], + // Without a payload there's no size field to look up, + // so the image falls back to a maximum size. + [ + { type: 'screenshot-data-url', sizeFieldForAspectRatio: 'windowSize' }, + 0, + ], ]; - const stringTable = StringTable.withBackingArray([]); + const stringTable = StringTable.withBackingArray([ + 'data:image/jpeg;base64,AAAA', + ]); expect( entries.map(([format, value]: [MarkerFormatType, any]): string[][] => [ format, @@ -604,6 +614,17 @@ describe('marker schema formatting', function () { }); }); +describe('computeStringIndexMarkerFieldsByDataType', function () { + it('treats the screenshot data URL as a string index', function () { + expect( + isStringIndexFormat({ + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }) + ).toBe(true); + }); +}); + describe('getMarkerSchema', function () { it('combines front-end and Gecko marker schema', function () { const { profile } = getProfileFromTextSamples('A'); diff --git a/src/types/markers.ts b/src/types/markers.ts index 7ee06d246a..9f05946ea5 100644 --- a/src/types/markers.ts +++ b/src/types/markers.ts @@ -86,7 +86,14 @@ export type MarkerFormatType = | 'pid' | 'tid' | 'list' - | { type: 'table'; columns: TableColumnFormat[] }; + // The size of a window in pixels, as a { width, height } object. + // "Label: 1280px × 1000px" + | 'screenshot-size' + | { type: 'table'; columns: TableColumnFormat[] } + // An image data URL, stored as an index into the profile's string table. + // It is rendered at the aspect ratio of the 'screenshot-size' field named by + // `sizeFieldForAspectRatio`. + | { type: 'screenshot-data-url'; sizeFieldForAspectRatio: string }; type TableColumnFormat = { // type for formatting, default is string From c73f9bb4a4ab4bce55005ca0ddae4714843cc0f7 Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 16:12:30 +0200 Subject: [PATCH 3/6] Normalize screenshot marker payloads Store window dimensions as one windowSize value so a schema field can describe them together. Normalize window IDs to strings at the profile boundary so consumers can use one representation regardless of whether Gecko supplies a numeric ID or a hexadecimal string. Apply the same payload shape in Gecko processing, Chrome import, and the processed profile upgrader. --- docs-developer/CHANGELOG-formats.md | 4 +- src/app-logic/constants.ts | 2 +- src/components/timeline/TrackScreenshots.tsx | 24 ++--- src/components/tooltip/Marker.tsx | 15 +-- src/profile-logic/import/chrome.ts | 3 +- src/profile-logic/marker-data.ts | 6 +- src/profile-logic/process-profile.ts | 11 +- .../processed-profile-versioning.ts | 20 ++++ src/test/components/TooltipMarker.test.tsx | 7 +- .../fixtures/profiles/processed-profile.ts | 7 +- src/test/fixtures/upgrades/processed-3.json | 40 ++++++- .../__snapshots__/profiler-edit.test.ts.snap | 8 +- .../__snapshots__/profile-view.test.ts.snap | 2 +- src/test/store/receive-profile.test.ts | 6 +- .../__snapshots__/marker-data.test.ts.snap | 6 +- .../profile-conversion.test.ts.snap | 36 +++---- .../profile-upgrading.test.ts.snap | 100 ++++++++++++------ src/test/unit/marker-data.test.ts | 66 +++++++++++- src/test/unit/merge-compare.test.ts | 9 +- .../unit/profile-query/marker-utils.test.ts | 8 +- src/types/markers.ts | 48 ++++----- 21 files changed, 278 insertions(+), 150 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index 82a15e43f5..8a3bc28755 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -8,7 +8,9 @@ Note that this is not an exhaustive list. Processed profile format upgraders can ### Version 76 -Two marker schema field formats were added: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. +The `CompositorScreenshot` marker payload's `windowWidth` and `windowHeight` fields were replaced with a single `windowSize` field of the form `{ width, height }`. The `windowID` field is now always a string. + +Two marker schema field formats were added to describe these markers: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. ### Version 75 diff --git a/src/app-logic/constants.ts b/src/app-logic/constants.ts index 6f9cae4e98..c1dea0ace1 100644 --- a/src/app-logic/constants.ts +++ b/src/app-logic/constants.ts @@ -12,7 +12,7 @@ export const GECKO_PROFILE_VERSION = 36; // The current version of the "processed" profile format. // Please don't forget to update the processed profile format changelog in // `docs-developer/CHANGELOG-formats.md`. -export const PROCESSED_PROFILE_VERSION = 75; +export const PROCESSED_PROFILE_VERSION = 76; // The following are the margin sizes for the left and right of the timeline. Independent // components need to share these values. diff --git a/src/components/timeline/TrackScreenshots.tsx b/src/components/timeline/TrackScreenshots.tsx index 4ec906de4e..282a55015d 100644 --- a/src/components/timeline/TrackScreenshots.tsx +++ b/src/components/timeline/TrackScreenshots.tsx @@ -244,19 +244,17 @@ class HoverPreview extends PureComponent { return null; } - if (payload.url === undefined) { + const { url, windowSize } = payload; + if (url === undefined || windowSize === undefined) { return null; } - const { url } = payload; - const maximumHoverSize = isMakingPreviewSelection ? MAXIMUM_HOVER_SIZE_WHEN_SELECTING_RANGE : MAXIMUM_HOVER_SIZE; - // Type guard: payload.url !== undefined means it has windowWidth and windowHeight const { width: hoverWidth, height: hoverHeight } = computeScreenshotSize( - payload as { windowWidth: number; windowHeight: number }, + windowSize, maximumHoverSize ); @@ -354,18 +352,12 @@ class ScreenshotStrip extends PureComponent { // Coerce the payload into a screenshot one. const payload: ScreenshotPayload = screenshots[screenshotIndex] .data as any; - if (payload.url === undefined) { + const { url: urlStringIndex, windowSize } = payload; + if (urlStringIndex === undefined || windowSize === undefined) { continue; } - const { - url: urlStringIndex, - windowWidth, - windowHeight, - } = payload as ScreenshotPayload & { - windowWidth: number; - windowHeight: number; - }; - const scaledImageWidth = (trackHeight * windowWidth) / windowHeight; + const scaledImageWidth = + (trackHeight * windowSize.width) / windowSize.height; images.push(
{ {/* The following image is centered and cropped by the outer container. */} { break; } case 'CompositorScreenshot': { - if ( - data.url !== undefined && - 'windowWidth' in data && - 'windowHeight' in data - ) { + if (data.url !== undefined && data.windowSize !== undefined) { const { width, height } = computeScreenshotSize( - data, + data.windowSize, MAXIMUM_IMAGE_SIZE ); details.push( @@ -380,7 +376,7 @@ class MarkerTooltipContents extends React.PureComponent { key="CompositorScreenshot-window size" > <> - {data.windowWidth}px × {data.windowHeight}px + {data.windowSize.width}px × {data.windowSize.height}px , { + // The CompositorScreenshot marker payload's `windowWidth` and + // `windowHeight` fields were replaced with a single `windowSize` field. + for (const thread of profile.threads) { + const { markers } = thread; + for (let i = 0; i < markers.length; i++) { + const data = markers.data[i]; + if (!data || data.type !== 'CompositorScreenshot') { + continue; + } + const { windowWidth, windowHeight } = data; + data.windowID = String(data.windowID); + if (windowWidth !== undefined && windowHeight !== undefined) { + data.windowSize = { width: windowWidth, height: windowHeight }; + } + delete data.windowWidth; + delete data.windowHeight; + } + } + }, // If you add a new upgrader here, please document the change in // `docs-developer/CHANGELOG-formats.md`. }; diff --git a/src/test/components/TooltipMarker.test.tsx b/src/test/components/TooltipMarker.test.tsx index c80c87ae87..5f2ccc7c24 100644 --- a/src/test/components/TooltipMarker.test.tsx +++ b/src/test/components/TooltipMarker.test.tsx @@ -1180,8 +1180,7 @@ describe('TooltipMarker', function () { type: 'CompositorScreenshot', url: screenshotUrlIndex, windowID: 'XXX', - windowWidth: 600, - windowHeight: 300, + windowSize: { width: 600, height: 300 }, }, ], ]); @@ -1213,7 +1212,6 @@ describe('TooltipMarker', function () { { type: 'CompositorScreenshot', windowID: 'XXX', - url: undefined, }, ], ]); @@ -1300,8 +1298,7 @@ describe('TooltipMarker', function () { type: 'CompositorScreenshot', url: screenshotUrlIndex, windowID: 'XXX', - windowWidth: 600, - windowHeight: 300, + windowSize: { width: 600, height: 300 }, }, ], ] diff --git a/src/test/fixtures/profiles/processed-profile.ts b/src/test/fixtures/profiles/processed-profile.ts index ffdca15c3a..9032ddd9d7 100644 --- a/src/test/fixtures/profiles/processed-profile.ts +++ b/src/test/fixtures/profiles/processed-profile.ts @@ -380,8 +380,7 @@ export function makeCompositorScreenshot( type: 'CompositorScreenshot', url: 0, windowID: '', - windowWidth: 100, - windowHeight: 100, + windowSize: { width: 100, height: 100 }, }, }; } @@ -1390,8 +1389,7 @@ export function getScreenshotMarkersForWindowId( type: 'CompositorScreenshot', url: 0, // Some arbitrary string. windowID, - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ]); } @@ -1408,7 +1406,6 @@ export function getScreenshotTrackProfile() { { type: 'CompositorScreenshot', windowID: '1', - url: undefined, }, ], ]); diff --git a/src/test/fixtures/upgrades/processed-3.json b/src/test/fixtures/upgrades/processed-3.json index df3eb93eaa..2594b4d624 100644 --- a/src/test/fixtures/upgrades/processed-3.json +++ b/src/test/fixtures/upgrades/processed-3.json @@ -347,30 +347,36 @@ ] }, "markers": { - "length": 10, + "length": 13, "name": [ 4, 5, 10, 10, + 18, 5, 17, 8, 8, 9, - 14 + 18, + 14, + 19 ], "time": [ 0, 2, 4, 5, + 6, 8, 8, 9, 10, 11, - 13 + 12, + 13, + 14 ], "data": [ { @@ -396,6 +402,13 @@ "interval": "end", "type": "tracing" }, + { + "type": "CompositorScreenshot", + "url": 20, + "windowID": 42, + "windowWidth": 1280, + "windowHeight": 1000 + }, { "category": "Paint", "interval": "end", @@ -428,6 +441,13 @@ "startTime": 11, "endTime": 12 }, + { + "type": "CompositorScreenshot", + "url": 20, + "windowID": "42", + "windowWidth": 1280, + "windowHeight": 1000 + }, { "type": "Network", "startTime": 3, @@ -446,6 +466,10 @@ "requestStart": 10, "responseStart": 11, "responseEnd": 12 + }, + { + "type": "CompositorScreenshot", + "windowID": 42 } ], "category": [ @@ -455,10 +479,13 @@ 4, 4, 4, + 4, 5, 5, 1, - 7 + 4, + 7, + 4 ] }, "samples": { @@ -527,7 +554,10 @@ "Load 32: https://github.com/rustwasm/wasm-bindgen/issues/3", "DiskIO", "FileIO", - "TextureCacheFree" + "TextureCacheFree", + "CompositorScreenshot", + "CompositorScreenshotWindowDestroyed", + "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAUD" ], "pid": "Unknown Process 1" }, diff --git a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap index f666a08608..4e7be1eef9 100644 --- a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap +++ b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap @@ -87,7 +87,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -1473,7 +1473,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -2859,7 +2859,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -4245,7 +4245,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "a.out", "sampleUnits": Object { diff --git a/src/test/store/__snapshots__/profile-view.test.ts.snap b/src/test/store/__snapshots__/profile-view.test.ts.snap index fa0a6575f8..a4bca85c8f 100644 --- a/src/test/store/__snapshots__/profile-view.test.ts.snap +++ b/src/test/store/__snapshots__/profile-view.test.ts.snap @@ -435,7 +435,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "Firefox", "sourceURL": "", diff --git a/src/test/store/receive-profile.test.ts b/src/test/store/receive-profile.test.ts index afc2309460..a768f16605 100644 --- a/src/test/store/receive-profile.test.ts +++ b/src/test/store/receive-profile.test.ts @@ -1757,8 +1757,7 @@ describe('actions/receive-profile', function () { type: 'CompositorScreenshot', url: 0, // Some arbitrary string. windowID: '0', - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ], ]); @@ -1771,8 +1770,7 @@ describe('actions/receive-profile', function () { type: 'CompositorScreenshot', url: 0, // Some arbitrary string. windowID: '1', - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ], ]); diff --git a/src/test/unit/__snapshots__/marker-data.test.ts.snap b/src/test/unit/__snapshots__/marker-data.test.ts.snap index df135ea0a7..344a6dbae1 100644 --- a/src/test/unit/__snapshots__/marker-data.test.ts.snap +++ b/src/test/unit/__snapshots__/marker-data.test.ts.snap @@ -179,9 +179,11 @@ Array [ "data": Object { "type": "CompositorScreenshot", "url": 21, - "windowHeight": 1000, "windowID": "0x136888400", - "windowWidth": 1280, + "windowSize": Object { + "height": 1000, + "width": 1280, + }, }, "end": 25, "name": "CompositorScreenshot", diff --git a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap index 4f9d85b213..6a63cf0ae9 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap @@ -42,7 +42,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -1022,7 +1022,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -2305,7 +2305,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -2697,7 +2697,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3086,7 +3086,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3187,7 +3187,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3540,7 +3540,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3605,7 +3605,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3759,7 +3759,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3817,7 +3817,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4207,7 +4207,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4265,7 +4265,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4323,7 +4323,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4643,7 +4643,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5019,7 +5019,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5319,7 +5319,7 @@ Object { "importedFrom": "dhat", "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "target/debug/examples/work_log (dhat)", "symbolicated": true, "version": 36, @@ -5452,7 +5452,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Flamegraph", "symbolicated": true, "version": 36, @@ -5510,7 +5510,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "product": "Flamegraph", "symbolicated": true, "version": 36, diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 6d77cbb21c..2243e7c71c 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -40,7 +40,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -7792,7 +7792,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -9210,7 +9210,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -10796,7 +10796,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 75, + "preprocessedProfileVersion": 76, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -11145,15 +11145,15 @@ Object { 0, ], "name": Array [ - 7, - 8, - 9, 10, 11, - 8, - 9, - 15, - 16, + 12, + 13, + 14, + 11, + 12, + 18, + 19, ], "originalLocation": Array [ 0, @@ -11210,10 +11210,10 @@ Object { 1, ], "name": Array [ - 8, - 9, - 8, - 9, + 11, + 12, + 11, + 12, ], }, "resourceTable": Object { @@ -11224,9 +11224,9 @@ Object { ], "length": 3, "name": Array [ - 13, - 12, - 14, + 16, + 15, + 17, ], "type": Array [ 1, @@ -11245,7 +11245,7 @@ Object { null, ], "filename": Array [ - 12, + 15, ], "id": Array [ null, @@ -11324,10 +11324,13 @@ Object { "VsyncTimestamp", "Reflow", "Rasterize", + "CompositorScreenshot", + "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAUD", "TextureCacheFree", "DOMEvent", "MinorGC", "Load 32: https://github.com/rustwasm/wasm-bindgen/issues/3", + "CompositorScreenshotWindowDestroyed", "(root)", "0x100000f84", "0x100001a45", @@ -11351,10 +11354,13 @@ Object { 4, 4, 4, + 4, 5, 5, 1, + 4, 7, + 4, ], "data": Array [ Object { @@ -11380,6 +11386,15 @@ Object { "interval": "end", "type": "tracing", }, + Object { + "type": "CompositorScreenshot", + "url": 4, + "windowID": "42", + "windowSize": Object { + "height": 1000, + "width": 1280, + }, + }, Object { "category": "Paint", "interval": "end", @@ -11407,6 +11422,15 @@ Object { "type": "DOMEvent", }, Object {}, + Object { + "type": "CompositorScreenshot", + "url": 4, + "windowID": "42", + "windowSize": Object { + "height": 1000, + "width": 1280, + }, + }, Object { "URI": "https://github.com/rustwasm/wasm-bindgen/issues/3", "connectEnd": 9, @@ -11426,55 +11450,71 @@ Object { "tcpConnectEnd": 7, "type": "Network", }, + Object { + "type": "CompositorScreenshot", + "windowID": "42", + }, ], "endTime": Array [ null, null, null, 5, + null, 8, null, null, 10, 12, + null, 13, + null, ], - "length": 10, + "length": 13, "name": Array [ 0, 1, 2, 2, - 1, 3, - 4, - 4, + 1, 5, 6, + 6, + 7, + 3, + 8, + 9, ], "phase": Array [ 0, 2, 2, 3, + 0, 3, 0, 2, 3, 1, + 0, 1, + 0, ], "startTime": Array [ 0, 2, 4, null, + 6, null, 8, 9, null, 11, + 12, 3, + 14, ], }, "name": "GeckoMain", @@ -11616,9 +11656,9 @@ Object { 2, 2, 1, - 4, - 4, - 5, + 6, + 6, + 7, ], "phase": Array [ 0, @@ -11802,10 +11842,10 @@ Object { 2, 2, 1, - 4, - 4, - 5, - 14, + 6, + 6, + 7, + 17, ], "phase": Array [ 0, diff --git a/src/test/unit/marker-data.test.ts b/src/test/unit/marker-data.test.ts index e8b42906f1..d01bb97fbf 100644 --- a/src/test/unit/marker-data.test.ts +++ b/src/test/unit/marker-data.test.ts @@ -325,12 +325,69 @@ describe('Derive markers from Gecko phase markers', function () { ]); }); + it.each([0, 42, '42', '0xAAAAAAAAA'])( + 'normalizes screenshot window ID %p to a string', + (windowID) => { + const screenshot = { + type: 'CompositorScreenshot' as const, + windowID, + url: 16, + windowWidth: 1280, + windowHeight: 1000, + }; + const { profile, markers } = setupWithTestDefinedMarkers([ + { + name: 'CompositorScreenshot', + startTime: 1, + endTime: null, + phase: INSTANT, + data: screenshot, + }, + { + name: 'CompositorScreenshot', + startTime: 2, + endTime: null, + phase: INSTANT, + data: { ...screenshot, windowID: String(windowID) }, + }, + { + name: 'CompositorScreenshotWindowDestroyed', + startTime: 3, + endTime: null, + phase: INSTANT, + data: { type: 'CompositorScreenshot', windowID }, + }, + ]); + + expect(profile.threads[0].markers.data).toEqual( + Array(3).fill( + expect.objectContaining({ + type: 'CompositorScreenshot', + windowID: String(windowID), + }) + ) + ); + expect(markers.slice(0, 2)).toEqual( + [1, 2].map((start) => + expect.objectContaining({ + name: 'CompositorScreenshot', + start, + end: start + 1, + data: expect.objectContaining({ + windowID: String(windowID), + windowSize: { width: 1280, height: 1000 }, + }), + }) + ) + ); + } + ); + it('has special handling for CompositorScreenshot', function () { const basePayload = { type: 'CompositorScreenshot' as const, url: 16, - windowWidth: 1280, - windowHeight: 1000, + windowSize: { width: 1280, height: 1000 }, }; const payloadsForWindowA: ScreenshotPayload[] = [ { @@ -339,7 +396,7 @@ describe('Derive markers from Gecko phase markers', function () { }, { ...basePayload, - windowWidth: 500, + windowSize: { width: 500, height: 1000 }, windowID: '0xAAAAAAAAA', }, ]; @@ -812,8 +869,7 @@ describe('deriveMarkersFromRawMarkerTable', function () { type: 'CompositorScreenshot', url: expect.anything(), windowID: '0x136888400', - windowWidth: 1280, - windowHeight: 1000, + windowSize: { width: 1280, height: 1000 }, }, name: 'CompositorScreenshot', start: 25, diff --git a/src/test/unit/merge-compare.test.ts b/src/test/unit/merge-compare.test.ts index 39e11e3abb..b7703880ee 100644 --- a/src/test/unit/merge-compare.test.ts +++ b/src/test/unit/merge-compare.test.ts @@ -384,8 +384,7 @@ describe('mergeProfilesForDiffing function', function () { type: 'CompositorScreenshot', url: stringTableB.indexForString(screenshotUrl), windowID: '0', - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ], ] @@ -791,8 +790,7 @@ describe('mergeThreads function', function () { type: 'CompositorScreenshot', url: screenshot1UrlIndex, windowID: 'XXX', - windowWidth: 300, - windowHeight: 600, + windowSize: { width: 300, height: 600 }, }, ], ]); @@ -806,8 +804,7 @@ describe('mergeThreads function', function () { type: 'CompositorScreenshot', url: screenshot2UrlIndex, windowID: 'YYY', - windowWidth: 300, - windowHeight: 600, + windowSize: { width: 300, height: 600 }, }, ], ]); diff --git a/src/test/unit/profile-query/marker-utils.test.ts b/src/test/unit/profile-query/marker-utils.test.ts index 10d4f7f8bb..18d333efb7 100644 --- a/src/test/unit/profile-query/marker-utils.test.ts +++ b/src/test/unit/profile-query/marker-utils.test.ts @@ -1022,8 +1022,7 @@ describe('collectThreadMarkers list option', function () { type: 'CompositorScreenshot', url: urlIdx, windowID: '0x1', - windowWidth: 1280, - windowHeight: 951, + windowSize: { width: 1280, height: 951 }, }); markers.length++; @@ -1059,7 +1058,7 @@ describe('collectThreadMarkers list option', function () { length: url.length, preview: url, }); - expect(m.data!.windowWidth).toBe(1280); + expect(m.data!.windowSize).toEqual({ width: 1280, height: 951 }); }); it('elides screenshot image blobs from data but keeps the key', function () { @@ -1089,8 +1088,7 @@ describe('collectThreadMarkers list option', function () { // The small fields next to it must survive: for this marker type there is // no schema, so `data` is the only route to the window dimensions. expect(m.fields).toBeUndefined(); - expect(m.data!.windowWidth).toBe(1280); - expect(m.data!.windowHeight).toBe(951); + expect(m.data!.windowSize).toEqual({ width: 1280, height: 951 }); // The whole row must stay small. expect(JSON.stringify(m).length).toBeLessThan(1000); }); diff --git a/src/types/markers.ts b/src/types/markers.ts index 9f05946ea5..5eb6cd9fd4 100644 --- a/src/types/markers.ts +++ b/src/types/markers.ts @@ -723,30 +723,28 @@ type VsyncTimestampPayload = { type: 'VsyncTimestamp'; }; -export type ScreenshotPayload = - | { - type: 'CompositorScreenshot'; - // This field represents the data url of the image. It is saved in the string table. - url: IndexIntoStringTable; - // A memory address that can uniquely identify a window. It has no meaning other than - // a way to identify a window. - windowID: string; - // The original dimensions of the window that was captured. The actual image that is - // stored in the string table will be scaled down from the original size. - windowWidth: number; - windowHeight: number; - } - // Markers that represent the closing of a window (name === 'CompositorScreenshotWindowDestroyed') - // only have a windowID data. - | { - type: 'CompositorScreenshot'; - // A memory address that can uniquely identify a window. It has no meaning other than - // a way to identify a window. - windowID: string; - // Having the property present but void makes it easier to deal with Flow in - // our flow version. - url: void; - }; +export type ScreenshotPayload = { + type: 'CompositorScreenshot'; + // A value that can uniquely identify a window. It has no meaning other than + // a way to identify a window. + windowID: string; + // This field represents the data url of the image. It is saved in the string table. + // The marker that only closes a window's last screenshot doesn't have one. + url?: IndexIntoStringTable; + // The original dimensions of the window that was captured. The actual image that is + // stored in the string table will be scaled down from the original size. + windowSize?: { width: number; height: number }; +}; + +export type ScreenshotPayload_Gecko = { + type: 'CompositorScreenshot'; + windowID: number | string; + url?: IndexIntoStringTable; + // Gecko writes the window dimensions as two separate fields. + // Profile processing folds them into a single `windowSize` field. + windowWidth?: number; + windowHeight?: number; +}; export type StyleMarkerPayload = { type: 'Styles'; @@ -920,7 +918,7 @@ export type MarkerPayload_Gecko = | GCMajorMarkerPayload_Gecko | GCSliceMarkerPayload_Gecko | VsyncTimestampPayload - | ScreenshotPayload + | ScreenshotPayload_Gecko | CcMarkerTracing | ArbitraryEventTracing | NavigationMarkerPayload From 77e22ea281bcbd21e0d397068457fdd42698a170 Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 16:14:36 +0200 Subject: [PATCH 4/6] Describe CompositorScreenshot markers with a persisted schema Persist screenshot field definitions with profiles so tooltip rendering, string-index translation, and query output can use the schema instead of separate CompositorScreenshot cases. Identify image URLs by their field format to keep query results compact. --- src/components/tooltip/Marker.tsx | 57 ------------------ src/profile-logic/import/chrome.ts | 12 ++++ src/profile-logic/marker-schema.ts | 4 -- src/profile-logic/process-profile.ts | 59 ++++++++++++------- .../process-screenshot-markers.ts | 27 +++++++++ .../processed-profile-versioning.ts | 30 ++++++++++ src/profile-query/formatters/marker-info.ts | 46 +++++++++------ src/test/components/TooltipMarker.test.tsx | 2 + .../__snapshots__/TooltipMarker.test.tsx.snap | 29 +++++---- .../fixtures/profiles/processed-profile.ts | 33 +++++++++++ .../profile-conversion.test.ts.snap | 2 + .../profile-upgrading.test.ts.snap | 28 +++++++++ src/test/unit/marker-schema.test.ts | 14 ++++- src/test/unit/merge-compare.test.ts | 7 +-- .../unit/profile-query/marker-utils.test.ts | 22 ++++--- 15 files changed, 247 insertions(+), 125 deletions(-) create mode 100644 src/profile-logic/process-screenshot-markers.ts diff --git a/src/components/tooltip/Marker.tsx b/src/components/tooltip/Marker.tsx index 60788caaf8..5d3fae8270 100644 --- a/src/components/tooltip/Marker.tsx +++ b/src/components/tooltip/Marker.tsx @@ -354,63 +354,6 @@ class MarkerTooltipContents extends React.PureComponent { ); break; } - case 'CompositorScreenshot': { - if (data.url !== undefined && data.windowSize !== undefined) { - const { width, height } = computeScreenshotSize( - data.windowSize, - MAXIMUM_IMAGE_SIZE - ); - details.push( - - - , - - <> - {data.windowSize.width}px × {data.windowSize.height}px - - , - - This marker spans the time between each composite of a window - and shows the window contents during that time. - , - - {data.windowID} - - ); - } else if (marker.name === 'CompositorScreenshotWindowDestroyed') { - details.push( - - This marker shows the moment a window has been destroyed. - , - - {data.windowID} - - ); - } - break; - } default: // Do nothing } diff --git a/src/profile-logic/import/chrome.ts b/src/profile-logic/import/chrome.ts index c8c6a292ac..7a9ad62b04 100644 --- a/src/profile-logic/import/chrome.ts +++ b/src/profile-logic/import/chrome.ts @@ -31,6 +31,7 @@ import { } from 'firefox-profiler/app-logic/constants'; import { getTimeRangeForThread } from '../profile-data'; +import { compositorScreenshotMarkerSchema } from '../process-screenshot-markers'; import { GlobalDataCollector } from '../global-data-collector'; // Chrome Tracing Event Spec: @@ -849,9 +850,18 @@ async function processTracingEvents( const { shared } = globalDataCollector.finish(); profile.shared = shared; + let hasScreenshots = false; for (const [thread, threadInfo] of threadInfoByThread) { thread.samples = finishRawSamplesTableBuilder(threadInfo.samples); thread.markers = finishRawMarkerTableBuilder(threadInfo.markers); + if ( + thread.markers.data.some((data) => data?.type === 'CompositorScreenshot') + ) { + hasScreenshots = true; + } + } + if (hasScreenshots) { + profile.meta.markerSchema.push(compositorScreenshotMarkerSchema); } return profile; @@ -897,6 +907,8 @@ async function extractScreenshots( markers.data.push({ type: 'CompositorScreenshot', url: stringTable.indexForString(urlString), + // Chrome's Screenshot events don't say which window they belong to, so + // they all share one made-up window ID. windowID: 'id', windowSize: { width: size.width, height: size.height }, }); diff --git a/src/profile-logic/marker-schema.ts b/src/profile-logic/marker-schema.ts index 10fd49bc7e..74e5a72e9d 100644 --- a/src/profile-logic/marker-schema.ts +++ b/src/profile-logic/marker-schema.ts @@ -784,10 +784,6 @@ export function computeStringIndexMarkerFieldsByDataType( ): Map { const stringIndexMarkerFieldsByDataType = new Map(); - // 'CompositorScreenshot' markers currently don't have a schema (#5303), - // hardcode the url field (which is a string index) until they do. - stringIndexMarkerFieldsByDataType.set('CompositorScreenshot', ['url']); - for (const schema of markerSchemas) { const { name, fields } = schema; const stringIndexFields = []; diff --git a/src/profile-logic/process-profile.ts b/src/profile-logic/process-profile.ts index 0e2d8e96a8..e2ba3af003 100644 --- a/src/profile-logic/process-profile.ts +++ b/src/profile-logic/process-profile.ts @@ -67,6 +67,7 @@ import { extensionTextMarkerSchema, } from '../profile-logic/marker-schema'; import { convertJsTracerToThread } from '../profile-logic/js-tracer'; +import { compositorScreenshotMarkerSchema } from './process-screenshot-markers'; import type { StringTable } from '../utils/string-table'; import type { @@ -1822,14 +1823,17 @@ function _convertGeckoMarkerSchema( * primary list that is stored on the processed profile's meta object. */ function processMarkerSchema(geckoProfile: GeckoProfile): MarkerSchema[] { - const combinedSchemas: MarkerSchema[] = geckoProfile.meta.markerSchema.map( - _convertGeckoMarkerSchema - ); + const combinedSchemas: MarkerSchema[] = geckoProfile.meta.markerSchema + .filter(({ name }) => name !== 'CompositorScreenshot') + .map(_convertGeckoMarkerSchema); const names: Set = new Set(combinedSchemas.map(({ name }) => name)); for (const subprocess of geckoProfile.processes) { for (const markerSchema of subprocess.meta.markerSchema) { - if (!names.has(markerSchema.name)) { + if ( + markerSchema.name !== 'CompositorScreenshot' && + !names.has(markerSchema.name) + ) { names.add(markerSchema.name); combinedSchemas.push(_convertGeckoMarkerSchema(markerSchema)); } @@ -1864,7 +1868,6 @@ function processMarkerSchema(geckoProfile: GeckoProfile): MarkerSchema[] { ) { combinedSchemas.push(extensionTextMarkerSchema); } - return combinedSchemas; } @@ -2024,7 +2027,10 @@ export function processGeckoProfile(geckoProfile: GeckoProfile): Profile { const markerSchema = processMarkerSchema(geckoProfile); const stringIndexMarkerFieldsByDataType = - computeStringIndexMarkerFieldsByDataType(markerSchema); + computeStringIndexMarkerFieldsByDataType([ + ...markerSchema, + compositorScreenshotMarkerSchema, + ]); const extensions: ExtensionTable = geckoProfile.meta.extensions ? _toStructOfArrays(geckoProfile.meta.extensions) @@ -2034,15 +2040,29 @@ export function processGeckoProfile(geckoProfile: GeckoProfile): Profile { globalDataCollector.addExtensionOrigins(extensions); - for (const thread of geckoProfile.threads) { - threads.push( - _processThread( - thread, - geckoProfile, - stringIndexMarkerFieldsByDataType, - globalDataCollector - ) + let hasCompositorScreenshots = false; + const processThread = ( + thread: GeckoThread, + processProfile: GeckoProfile | GeckoSubprocessProfile + ): RawThread => { + const newThread = _processThread( + thread, + processProfile, + stringIndexMarkerFieldsByDataType, + globalDataCollector ); + if ( + newThread.markers.data.some( + (data) => data?.type === 'CompositorScreenshot' + ) + ) { + hasCompositorScreenshots = true; + } + return newThread; + }; + + for (const thread of geckoProfile.threads) { + threads.push(processThread(thread, geckoProfile)); } const counters: RawCounter[] = _processCounters(geckoProfile, threads, 0); const nullableProfilerOverhead: Array = [ @@ -2053,12 +2073,7 @@ export function processGeckoProfile(geckoProfile: GeckoProfile): Profile { const adjustTimestampsBy = subprocessProfile.meta.startTime - geckoProfile.meta.startTime; for (const thread of subprocessProfile.threads) { - const newThread: RawThread = _processThread( - thread, - subprocessProfile, - stringIndexMarkerFieldsByDataType, - globalDataCollector - ); + const newThread = processThread(thread, subprocessProfile); newThread.samples = adjustTableTimeDeltas( newThread.samples, adjustTimestampsBy @@ -2105,6 +2120,10 @@ export function processGeckoProfile(geckoProfile: GeckoProfile): Profile { ); } + if (hasCompositorScreenshots) { + markerSchema.push(compositorScreenshotMarkerSchema); + } + let pages = [...(geckoProfile.pages || [])]; for (const subprocessProfile of geckoProfile.processes) { diff --git a/src/profile-logic/process-screenshot-markers.ts b/src/profile-logic/process-screenshot-markers.ts new file mode 100644 index 0000000000..dc39bc7d1b --- /dev/null +++ b/src/profile-logic/process-screenshot-markers.ts @@ -0,0 +1,27 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { oneLine } from 'common-tags'; +import type { MarkerSchema } from 'firefox-profiler/types'; + +export const compositorScreenshotMarkerSchema: MarkerSchema = { + name: 'CompositorScreenshot', + display: ['marker-chart', 'marker-table'], + fields: [ + { + key: 'url', + label: 'Image', + format: { + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }, + }, + { key: 'windowSize', label: 'Window Size', format: 'screenshot-size' }, + { key: 'windowID', label: 'Window ID', format: 'string' }, + ], + description: oneLine` + This marker spans the time between each composite of a window and shows + the window contents during that time. + `, +}; diff --git a/src/profile-logic/processed-profile-versioning.ts b/src/profile-logic/processed-profile-versioning.ts index f07b10463e..426477a9ab 100644 --- a/src/profile-logic/processed-profile-versioning.ts +++ b/src/profile-logic/processed-profile-versioning.ts @@ -3673,6 +3673,7 @@ const _upgraders: { [76]: (profile: any) => { // The CompositorScreenshot marker payload's `windowWidth` and // `windowHeight` fields were replaced with a single `windowSize` field. + let hasCompositorScreenshots = false; for (const thread of profile.threads) { const { markers } = thread; for (let i = 0; i < markers.length; i++) { @@ -3681,6 +3682,7 @@ const _upgraders: { continue; } const { windowWidth, windowHeight } = data; + hasCompositorScreenshots = true; data.windowID = String(data.windowID); if (windowWidth !== undefined && windowHeight !== undefined) { data.windowSize = { width: windowWidth, height: windowHeight }; @@ -3689,6 +3691,34 @@ const _upgraders: { delete data.windowHeight; } } + + profile.meta.markerSchema = profile.meta.markerSchema.filter( + ({ name }: any) => name !== 'CompositorScreenshot' + ); + if (hasCompositorScreenshots) { + profile.meta.markerSchema.push({ + name: 'CompositorScreenshot', + display: ['marker-chart', 'marker-table'], + fields: [ + { + key: 'url', + label: 'Image', + format: { + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }, + }, + { + key: 'windowSize', + label: 'Window Size', + format: 'screenshot-size', + }, + { key: 'windowID', label: 'Window ID', format: 'string' }, + ], + description: + 'This marker spans the time between each composite of a window and shows the window contents during that time.', + }); + } }, // If you add a new upgrader here, please document the change in // `docs-developer/CHANGELOG-formats.md`. diff --git a/src/profile-query/formatters/marker-info.ts b/src/profile-query/formatters/marker-info.ts index b16259ef84..57f3e2e5b0 100644 --- a/src/profile-query/formatters/marker-info.ts +++ b/src/profile-query/formatters/marker-info.ts @@ -42,6 +42,7 @@ import type { Lib, IndexIntoStackTable, MarkerSchemaByName, + MarkerFormatType, } from 'firefox-profiler/types'; import type { StringTable } from 'firefox-profiler/utils/string-table'; import type { @@ -891,6 +892,7 @@ export function collectThreadMarkers( const label = getMarkerLabel(markerIndex); const data = collectMarkerData( marker, + markerSchemaByName, stringIndexFieldsByDataType, stringTable ); @@ -1068,6 +1070,7 @@ export function collectProfileMarkers( ), data: collectMarkerData( marker, + markerSchemaByName, stringIndexFieldsByDataType, stringTable ), @@ -1306,7 +1309,7 @@ function collectMarkerFields( fields.push({ key: field.key, label: field.label || field.key, - value, + value: truncateScreenshotDataUrl(value, field.format), formattedValue, }); } @@ -1315,21 +1318,33 @@ function collectMarkerFields( return fields; } -// Image blobs, elided from `data` because `--list` repeats them on every row. -// Keyed by field, not by size: a 2855-char screenshot data URL sits *below* a -// legitimate 2939-char `prefValue`, so no byte cap separates the two. -const ELIDED_PAYLOAD_FIELDS: ReadonlyMap> = new Map( - [['CompositorScreenshot', new Set(['url'])]] -); - export type ElidedDataValue = { elided: true; length: number; preview: string; }; +function truncateScreenshotDataUrl( + resolvedValue: unknown, + format: MarkerFormatType | undefined +): unknown { + if ( + typeof format === 'object' && + format.type === 'screenshot-data-url' && + typeof resolvedValue === 'string' + ) { + return { + elided: true, + length: resolvedValue.length, + preview: resolvedValue.slice(0, 64), + } satisfies ElidedDataValue; + } + return resolvedValue; +} + function collectMarkerData( marker: Marker, + markerSchemaByName: MarkerSchemaByName, stringIndexFieldsByDataType: Map, stringTable: StringTable ): { [key: string]: any } | undefined { @@ -1339,7 +1354,7 @@ function collectMarkerData( } const stringIndexFields = stringIndexFieldsByDataType.get(payload.type); - const elidedFields = ELIDED_PAYLOAD_FIELDS.get(payload.type); + const schema = markerSchemaByName[payload.type]; const data: { [key: string]: any } = {}; for (const [key, value] of Object.entries(payload)) { if (OMITTED_PAYLOAD_KEYS.has(key) || value === undefined) { @@ -1349,15 +1364,10 @@ function collectMarkerData( stringIndexFields?.includes(key) && typeof value === 'number' ? stringTable.getString(value, '(empty)') : value; - if (elidedFields?.has(key) && typeof resolved === 'string') { - data[key] = { - elided: true, - length: resolved.length, - preview: resolved.slice(0, 64), - } satisfies ElidedDataValue; - } else { - data[key] = resolved; - } + data[key] = truncateScreenshotDataUrl( + resolved, + schema?.fields.find((field) => field.key === key)?.format + ); } // A payload of only `type` yields no keys here; report nothing. diff --git a/src/test/components/TooltipMarker.test.tsx b/src/test/components/TooltipMarker.test.tsx index 5f2ccc7c24..e18af68e04 100644 --- a/src/test/components/TooltipMarker.test.tsx +++ b/src/test/components/TooltipMarker.test.tsx @@ -12,6 +12,7 @@ import { import { TooltipMarker } from '../../components/tooltip/Marker'; import { storeWithProfile } from '../fixtures/stores'; import { + addCompositorScreenshotSchemaIfNeeded, addMarkersToThreadWithCorrespondingSamples, getProfileFromTextSamples, getNetworkMarkers, @@ -1184,6 +1185,7 @@ describe('TooltipMarker', function () { }, ], ]); + addCompositorScreenshotSchemaIfNeeded(profile); const store = storeWithProfile(profile); const { getState } = store; diff --git a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap index 011f064231..19fbb30030 100644 --- a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap +++ b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap @@ -2037,7 +2037,11 @@ exports[`TooltipMarker renders the tooltip of CompositorScreenshotWindowDestroye Description :
- This marker shows the moment a window has been destroyed. +
+ This marker spans the time between each composite of a window and shows the window contents during that time. +
@@ -5189,6 +5193,17 @@ exports[`TooltipMarker shows image of CompositorScreenshot markers 1`] = `
+
+ Description + : +
+
+ This marker spans the time between each composite of a window and shows the window contents during that time. +
@@ -5206,17 +5221,7 @@ exports[`TooltipMarker shows image of CompositorScreenshot markers 1`] = ` Window Size :
- 600 - px × - 300 - px -
- Description - : -
- This marker spans the time between each composite of a window and shows the window contents during that time. + 600px × 300px
diff --git a/src/test/fixtures/profiles/processed-profile.ts b/src/test/fixtures/profiles/processed-profile.ts index 9032ddd9d7..6e24047412 100644 --- a/src/test/fixtures/profiles/processed-profile.ts +++ b/src/test/fixtures/profiles/processed-profile.ts @@ -420,9 +420,42 @@ export function getProfileWithMarkers( ...getThreadWithMarkers(profile.shared, testDefinedMarkers), tid: i, })); + addCompositorScreenshotSchemaIfNeeded(profile); return profile; } +export const compositorScreenshotMarkerSchema: MarkerSchema = { + name: 'CompositorScreenshot', + display: ['marker-chart', 'marker-table'], + fields: [ + { + key: 'url', + label: 'Image', + format: { + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }, + }, + { key: 'windowSize', label: 'Window Size', format: 'screenshot-size' }, + { key: 'windowID', label: 'Window ID', format: 'string' }, + ], + description: + 'This marker spans the time between each composite of a window and shows the window contents during that time.', +}; + +export function addCompositorScreenshotSchemaIfNeeded(profile: Profile): void { + if ( + profile.threads.some((thread) => + thread.markers.data.some((data) => data?.type === 'CompositorScreenshot') + ) + ) { + profile.meta.markerSchema = [ + ...profile.meta.markerSchema, + compositorScreenshotMarkerSchema, + ]; + } +} + /** * This profile is useful for marker table tests. The markers were taken from * real-world values. diff --git a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap index 6a63cf0ae9..f29aea1813 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap @@ -2304,6 +2304,7 @@ Object { "interval": 0.5, "markerSchemaNames": Array [ "EventDispatch", + "CompositorScreenshot", ], "preprocessedProfileVersion": 76, "product": "Chrome Trace", @@ -2696,6 +2697,7 @@ Object { "interval": 0.5, "markerSchemaNames": Array [ "EventDispatch", + "CompositorScreenshot", ], "preprocessedProfileVersion": 76, "product": "Chrome Trace", diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 2243e7c71c..80c48726c1 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -10792,6 +10792,34 @@ Object { ], "name": "tracing", }, + Object { + "description": "This marker spans the time between each composite of a window and shows the window contents during that time.", + "display": Array [ + "marker-chart", + "marker-table", + ], + "fields": Array [ + Object { + "format": Object { + "sizeFieldForAspectRatio": "windowSize", + "type": "screenshot-data-url", + }, + "key": "url", + "label": "Image", + }, + Object { + "format": "screenshot-size", + "key": "windowSize", + "label": "Window Size", + }, + Object { + "format": "string", + "key": "windowID", + "label": "Window ID", + }, + ], + "name": "CompositorScreenshot", + }, ], "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", diff --git a/src/test/unit/marker-schema.test.ts b/src/test/unit/marker-schema.test.ts index dabe96ea04..c1e5e81b06 100644 --- a/src/test/unit/marker-schema.test.ts +++ b/src/test/unit/marker-schema.test.ts @@ -9,6 +9,7 @@ import { extensionTextMarkerSchema, markerSchemaFrontEndOnly, isStringIndexFormat, + computeStringIndexMarkerFieldsByDataType, } from '../../profile-logic/marker-schema'; import { renderMarkerFieldValue } from 'firefox-profiler/components/tooltip/Marker'; import type { @@ -19,7 +20,10 @@ import type { import { getDefaultCategories } from '../../profile-logic/data-structures'; import { storeWithProfile } from '../fixtures/stores'; import { getMarkerSchema } from '../../selectors/profile'; -import { getProfileFromTextSamples } from '../fixtures/profiles/processed-profile'; +import { + getProfileFromTextSamples, + compositorScreenshotMarkerSchema, +} from '../fixtures/profiles/processed-profile'; import { markerSchemaForTests } from '../fixtures/profiles/marker-schema'; import { StringTable } from '../../utils/string-table'; @@ -623,6 +627,14 @@ describe('computeStringIndexMarkerFieldsByDataType', function () { }) ).toBe(true); }); + + it('finds screenshot data URLs in the screenshot marker schema', function () { + expect( + computeStringIndexMarkerFieldsByDataType([ + compositorScreenshotMarkerSchema, + ]) + ).toEqual(new Map([['CompositorScreenshot', ['url']]])); + }); }); describe('getMarkerSchema', function () { diff --git a/src/test/unit/merge-compare.test.ts b/src/test/unit/merge-compare.test.ts index b7703880ee..c2b7624b53 100644 --- a/src/test/unit/merge-compare.test.ts +++ b/src/test/unit/merge-compare.test.ts @@ -11,6 +11,7 @@ import { stateFromLocation } from '../../app-logic/url-handling'; import { getProfileFromTextSamples, getProfileWithMarkers, + addCompositorScreenshotSchemaIfNeeded, addMarkersToThreadWithCorrespondingSamples, } from '../fixtures/profiles/processed-profile'; import { markerSchemaForTests } from '../fixtures/profiles/marker-schema'; @@ -389,11 +390,7 @@ describe('mergeProfilesForDiffing function', function () { ], ] ); - profileB.meta.markerSchema.push({ - name: 'CompositorScreenshot', - display: ['marker-chart', 'marker-table'], - fields: [{ key: 'url', label: 'Image', format: 'unique-string' }], - }); + addCompositorScreenshotSchemaIfNeeded(profileB); const profileState = stateFromLocation({ pathname: '/public/fakehash1/', diff --git a/src/test/unit/profile-query/marker-utils.test.ts b/src/test/unit/profile-query/marker-utils.test.ts index 18d333efb7..4d6530b398 100644 --- a/src/test/unit/profile-query/marker-utils.test.ts +++ b/src/test/unit/profile-query/marker-utils.test.ts @@ -23,6 +23,7 @@ import { getProfileWithMarkers, getProfileFromTextSamples, getNetworkMarkers, + compositorScreenshotMarkerSchema, } from '../../fixtures/profiles/processed-profile'; import type { NetworkMarkersOptions, @@ -998,13 +999,14 @@ describe('collectThreadMarkers list option', function () { * Build a profile with one CompositorScreenshot marker whose `url` is a real * index into the string table, rather than an already-resolved string. * - * Index resolution is driven by the marker schema: only fields the schema - * declares as unique-string are looked up. `markerSchemaForTests` has no - * CompositorScreenshot entry, so `url` reaches the output as the raw index - * and the elision path sees it unresolved. + * The screenshot schema identifies `url` as a string-table index holding image data. */ function setupWithScreenshotMarker(url: string) { const { profile } = getProfileFromTextSamples('someFunc'); + profile.meta.markerSchema = [ + ...profile.meta.markerSchema, + compositorScreenshotMarkerSchema, + ]; const thread = profile.threads[0]; const stringTable = StringTable.withBackingArray( profile.shared.stringArray @@ -1012,7 +1014,6 @@ describe('collectThreadMarkers list option', function () { const urlIdx = stringTable.indexForString(url); const markerNameIdx = stringTable.indexForString('CompositorScreenshot'); const markers = getRawMarkerTableBuilderFromExisting(thread.markers); - thread.markers = markers; markers.name.push(markerNameIdx); markers.startTime.push(1); markers.endTime.push(null); @@ -1025,6 +1026,7 @@ describe('collectThreadMarkers list option', function () { windowSize: { width: 1280, height: 951 }, }); markers.length++; + thread.markers = finishRawMarkerTableBuilder(markers); const store = storeWithProfile(profile); const threadMap = new ThreadMap(); @@ -1085,9 +1087,13 @@ describe('collectThreadMarkers list option', function () { length: url.length, preview: url.slice(0, 64), }); - // The small fields next to it must survive: for this marker type there is - // no schema, so `data` is the only route to the window dimensions. - expect(m.fields).toBeUndefined(); + expect(m.fields!.find(({ key }) => key === 'url')!.value).toEqual( + m.data!.url + ); + expect(m.fields!.find(({ key }) => key === 'windowSize')!.value).toEqual({ + width: 1280, + height: 951, + }); expect(m.data!.windowSize).toEqual({ width: 1280, height: 951 }); // The whole row must stay small. expect(JSON.stringify(m).length).toBeLessThan(1000); From 6c557a829d8e7f0b597f51f2da17afb9a5a22586 Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 16:16:46 +0200 Subject: [PATCH 5/6] Store screenshot markers as start and end marker pairs Encode screenshot lifetimes as start and end marker pairs and include the window ID in their names. Generic interval derivation can then match screenshots without its own per-window bookkeeping. Window destruction closes the last screenshot. An unclosed window leaves an incomplete interval extending to the end of the thread. --- docs-developer/CHANGELOG-formats.md | 2 + src/profile-logic/import/chrome.ts | 14 +- src/profile-logic/marker-data.ts | 68 ---------- src/profile-logic/process-profile.ts | 16 ++- .../process-screenshot-markers.ts | 103 ++++++++++++++- .../processed-profile-versioning.ts | 95 +++++++++++++- src/profile-logic/tracks.ts | 36 +++--- src/test/components/TooltipMarker.test.tsx | 16 --- src/test/components/TrackContextMenu.test.tsx | 6 +- src/test/components/TrackScreenshots.test.tsx | 15 +-- .../__snapshots__/TooltipMarker.test.tsx.snap | 70 +--------- .../TrackScreenshots.test.tsx.snap | 22 ++-- .../fixtures/profiles/processed-profile.ts | 83 +++++++----- src/test/store/profile-view.test.ts | 8 +- .../__snapshots__/marker-data.test.ts.snap | 3 +- .../profile-conversion.test.ts.snap | 12 +- .../profile-upgrading.test.ts.snap | 28 ++-- src/test/unit/marker-data.test.ts | 120 ++++++++++++------ 18 files changed, 416 insertions(+), 301 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index 8a3bc28755..e0332a2e96 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -10,6 +10,8 @@ Note that this is not an exhaustive list. Processed profile format upgraders can The `CompositorScreenshot` marker payload's `windowWidth` and `windowHeight` fields were replaced with a single `windowSize` field of the form `{ width, height }`. The `windowID` field is now always a string. +These markers are now stored as pairs of start and end markers instead of instant markers. The window ID is part of the marker name (`CompositorScreenshot `), so starts and ends are matched by name like any other marker pair. The `CompositorScreenshotWindowDestroyed` marker is gone: it is now the end marker of that window's last screenshot. The last screenshot of a window that is never destroyed has no end marker, and is extended to the end of the thread. + Two marker schema field formats were added to describe these markers: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. ### Version 75 diff --git a/src/profile-logic/import/chrome.ts b/src/profile-logic/import/chrome.ts index 7a9ad62b04..71f4318e85 100644 --- a/src/profile-logic/import/chrome.ts +++ b/src/profile-logic/import/chrome.ts @@ -31,7 +31,10 @@ import { } from 'firefox-profiler/app-logic/constants'; import { getTimeRangeForThread } from '../profile-data'; -import { compositorScreenshotMarkerSchema } from '../process-screenshot-markers'; +import { + compositorScreenshotMarkerSchema, + convertScreenshotMarkersToStartEnd, +} from '../process-screenshot-markers'; import { GlobalDataCollector } from '../global-data-collector'; // Chrome Tracing Event Spec: @@ -854,10 +857,13 @@ async function processTracingEvents( for (const [thread, threadInfo] of threadInfoByThread) { thread.samples = finishRawSamplesTableBuilder(threadInfo.samples); thread.markers = finishRawMarkerTableBuilder(threadInfo.markers); - if ( - thread.markers.data.some((data) => data?.type === 'CompositorScreenshot') - ) { + const markers = convertScreenshotMarkersToStartEnd( + thread.markers, + stringTable + ); + if (markers !== null) { hasScreenshots = true; + thread.markers = markers; } } if (hasScreenshots) { diff --git a/src/profile-logic/marker-data.ts b/src/profile-logic/marker-data.ts index 669fb11840..0f63eea0d7 100644 --- a/src/profile-logic/marker-data.ts +++ b/src/profile-logic/marker-data.ts @@ -563,7 +563,6 @@ export function correlateIPCMarkers( * the thread range. For the reverse situation, it's set to the start. * * There is also some special handling of different markers. - * - CompositorScreenshot - They are turned from Instant markers to Interval markers * - IPC - They are matched up. * - Network - They have different network phases. * @@ -621,10 +620,6 @@ export function deriveMarkersFromRawMarkerTable( } as MarkerPayload; } - // We don't add a screenshot marker as we find it, because to know its - // duration we need to wait until the next one or the end of the profile. So - // we keep it here. - const previousScreenshotMarkers: Map = new Map(); for ( let rawMarkerIndex = 0; rawMarkerIndex < rawMarkers.length; @@ -732,51 +727,6 @@ export function deriveMarkersFromRawMarkerTable( continue; } - case 'CompositorScreenshot': { - // Screenshot markers are already ordered. In the raw marker table, - // they're Instant markers, but since they're valid until the following - // raw marker of the same type and the same window, we convert them to - // Interval markers with a a start and end time. - - const { windowID } = data; - const previousScreenshotMarker = - previousScreenshotMarkers.get(windowID); - if (previousScreenshotMarker !== undefined) { - previousScreenshotMarkers.delete(windowID); - const previousStartTime = ensureExists( - rawMarkers.startTime[previousScreenshotMarker], - 'Expected to find a start time for a screenshot marker.' - ); - const thisStartTime = ensureExists( - maybeStartTime, - 'The CompositorScreenshot is assumed to have a start time.' - ); - const data = rawMarkers.data[previousScreenshotMarker]; - const markerThreadId = rawMarkers.threadId - ? rawMarkers.threadId[previousScreenshotMarker] - : null; - addMarker([previousScreenshotMarker], { - start: previousStartTime, - end: thisStartTime, - name: 'CompositorScreenshot', - category, - threadId: markerThreadId, - data, - }); - } - if (stringArray[name] === 'CompositorScreenshotWindowDestroyed') { - // This marker is added when a window is destroyed. In this case we - // don't want to store it as the start of the next compositor - // marker. But we do want to keep it, so we break out of the - // switch/case so that the standard processing happens. - break; - } else { - previousScreenshotMarkers.set(windowID, rawMarkerIndex); - } - - continue; - } - case 'IPC': { const sharedData = ipcCorrelations.get( // Older profiles don't have a tid, but they also don't have the IPC markers. @@ -994,24 +944,6 @@ export function deriveMarkersFromRawMarkerTable( }); } - // And we also need to add the "last screenshot markers". - for (const previousScreenshotMarker of previousScreenshotMarkers.values()) { - const start = ensureExists( - rawMarkers.startTime[previousScreenshotMarker], - 'Expected to find a CompositorScreenshot marker with a start time.' - ); - addMarker([previousScreenshotMarker], { - start, - end: Math.max(endOfThread, start), - name: 'CompositorScreenshot', - category: rawMarkers.category[previousScreenshotMarker], - threadId: rawMarkers.threadId - ? rawMarkers.threadId[previousScreenshotMarker] - : null, - data: rawMarkers.data[previousScreenshotMarker], - }); - } - return { markers, markerIndexToRawMarkerIndexes }; } diff --git a/src/profile-logic/process-profile.ts b/src/profile-logic/process-profile.ts index e2ba3af003..d72ab76f74 100644 --- a/src/profile-logic/process-profile.ts +++ b/src/profile-logic/process-profile.ts @@ -67,7 +67,10 @@ import { extensionTextMarkerSchema, } from '../profile-logic/marker-schema'; import { convertJsTracerToThread } from '../profile-logic/js-tracer'; -import { compositorScreenshotMarkerSchema } from './process-screenshot-markers'; +import { + compositorScreenshotMarkerSchema, + convertScreenshotMarkersToStartEnd, +} from './process-screenshot-markers'; import type { StringTable } from '../utils/string-table'; import type { @@ -2051,12 +2054,13 @@ export function processGeckoProfile(geckoProfile: GeckoProfile): Profile { stringIndexMarkerFieldsByDataType, globalDataCollector ); - if ( - newThread.markers.data.some( - (data) => data?.type === 'CompositorScreenshot' - ) - ) { + const markers = convertScreenshotMarkersToStartEnd( + newThread.markers, + globalDataCollector.getStringTable() + ); + if (markers !== null) { hasCompositorScreenshots = true; + newThread.markers = markers; } return newThread; }; diff --git a/src/profile-logic/process-screenshot-markers.ts b/src/profile-logic/process-screenshot-markers.ts index dc39bc7d1b..311751e26d 100644 --- a/src/profile-logic/process-screenshot-markers.ts +++ b/src/profile-logic/process-screenshot-markers.ts @@ -3,7 +3,21 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import { oneLine } from 'common-tags'; -import type { MarkerSchema } from 'firefox-profiler/types'; +import { INTERVAL_START, INTERVAL_END } from '../app-logic/constants'; +import { + getRawMarkerTableBuilder, + finishRawMarkerTableBuilder, +} from './data-structures'; +import { ensureExists } from '../utils/types'; +import type { StringTable } from '../utils/string-table'; +import type { + IndexIntoStringTable, + MarkerPayload, + MarkerPhase, + MarkerSchema, + Milliseconds, + RawMarkerTable, +} from 'firefox-profiler/types'; export const compositorScreenshotMarkerSchema: MarkerSchema = { name: 'CompositorScreenshot', @@ -25,3 +39,90 @@ export const compositorScreenshotMarkerSchema: MarkerSchema = { the window contents during that time. `, }; + +/** + * Gecko emits one instant marker per composite of a window, plus a + * CompositorScreenshotWindowDestroyed marker when the window goes away. + * Each screenshot is valid until the next one for the same window, so rewrite + * them into start / end marker pairs. + * A window that is never destroyed keeps its last screenshot open, + * so that marker gets extended to the end of the thread. + * + * Returns null when the table has no screenshot markers. + */ +export function convertScreenshotMarkersToStartEnd( + markers: RawMarkerTable, + stringTable: StringTable +): RawMarkerTable | null { + const hasScreenshots = markers.data.some( + (data) => data !== null && data.type === 'CompositorScreenshot' + ); + if (!hasScreenshots) { + return null; + } + + const newMarkers = getRawMarkerTableBuilder(); + const { threadId } = markers; + if (threadId !== undefined) { + newMarkers.threadId = []; + } + const openWindows = new Set(); + + function push( + name: IndexIntoStringTable, + startTime: Milliseconds | null, + endTime: Milliseconds | null, + phase: MarkerPhase, + sourceIndex: number, + data: MarkerPayload | null + ) { + newMarkers.name.push(name); + newMarkers.startTime.push(startTime); + newMarkers.endTime.push(endTime); + newMarkers.phase.push(phase); + newMarkers.category.push(markers.category[sourceIndex]); + newMarkers.data.push(data); + if (threadId !== undefined) { + ensureExists(newMarkers.threadId).push(threadId[sourceIndex]); + } + newMarkers.length++; + } + + for (let i = 0; i < markers.length; i++) { + const data = markers.data[i]; + if (data === null || data.type !== 'CompositorScreenshot') { + push( + markers.name[i], + markers.startTime[i], + markers.endTime[i], + markers.phase[i], + i, + data + ); + continue; + } + + const { windowID } = data; + const time = markers.startTime[i]; + const name = stringTable.indexForString(`CompositorScreenshot ${windowID}`); + const isWindowDestroyed = + stringTable.getString(markers.name[i]) === + 'CompositorScreenshotWindowDestroyed'; + if (openWindows.delete(windowID) || isWindowDestroyed) { + // The end marker carries the payload type so that code walking the raw table + // can recognize it during sanitization. + push(name, null, time, INTERVAL_END, i, { + type: 'CompositorScreenshot', + windowID, + }); + } + if (isWindowDestroyed) { + continue; + } + + push(name, time, null, INTERVAL_START, i, data); + openWindows.add(windowID); + } + + return finishRawMarkerTableBuilder(newMarkers); +} diff --git a/src/profile-logic/processed-profile-versioning.ts b/src/profile-logic/processed-profile-versioning.ts index 426477a9ab..7dd1988d34 100644 --- a/src/profile-logic/processed-profile-versioning.ts +++ b/src/profile-logic/processed-profile-versioning.ts @@ -3672,24 +3672,105 @@ const _upgraders: { }, [76]: (profile: any) => { // The CompositorScreenshot marker payload's `windowWidth` and - // `windowHeight` fields were replaced with a single `windowSize` field. + // `windowHeight` fields were replaced with a single `windowSize` field, and + // these markers are now stored as start / end marker pairs whose name + // carries the window ID, instead of instant markers. + // The CompositorScreenshotWindowDestroyed marker becomes the end marker of + // that window's last screenshot. + const INTERVAL_START = 2; + const INTERVAL_END = 3; + const stringTable = StringTable.withBackingArray( + profile.shared.stringArray + ); + let hasCompositorScreenshots = false; for (const thread of profile.threads) { const { markers } = thread; + if ( + !markers.data.some((data: any) => data?.type === 'CompositorScreenshot') + ) { + continue; + } + hasCompositorScreenshots = true; + + const newMarkers: any = { + data: [], + name: [], + startTime: [], + endTime: [], + phase: [], + category: [], + length: 0, + }; + const hasThreadId = Boolean(markers.threadId); + if (hasThreadId) { + newMarkers.threadId = []; + } + + const push = ( + name: number, + startTime: number | null, + endTime: number | null, + phase: number, + sourceIndex: number, + data: any + ) => { + newMarkers.name.push(name); + newMarkers.startTime.push(startTime); + newMarkers.endTime.push(endTime); + newMarkers.phase.push(phase); + newMarkers.category.push(markers.category[sourceIndex]); + newMarkers.data.push(data); + if (hasThreadId) { + newMarkers.threadId.push(markers.threadId[sourceIndex]); + } + newMarkers.length++; + }; + + const openWindows = new Set(); for (let i = 0; i < markers.length; i++) { const data = markers.data[i]; if (!data || data.type !== 'CompositorScreenshot') { + push( + markers.name[i], + markers.startTime[i], + markers.endTime[i], + markers.phase[i], + i, + data + ); continue; } - const { windowWidth, windowHeight } = data; - hasCompositorScreenshots = true; - data.windowID = String(data.windowID); + + const windowID = String(data.windowID); + const time = markers.startTime[i]; + const name = stringTable.indexForString( + `CompositorScreenshot ${windowID}` + ); + const isWindowDestroyed = + stringTable.getString(markers.name[i]) === + 'CompositorScreenshotWindowDestroyed'; + if (openWindows.delete(windowID) || isWindowDestroyed) { + push(name, null, time, INTERVAL_END, i, { + type: 'CompositorScreenshot', + windowID, + }); + } + if (isWindowDestroyed) { + continue; + } + + const { windowWidth, windowHeight, ...startData } = data; + startData.windowID = windowID; if (windowWidth !== undefined && windowHeight !== undefined) { - data.windowSize = { width: windowWidth, height: windowHeight }; + startData.windowSize = { width: windowWidth, height: windowHeight }; } - delete data.windowWidth; - delete data.windowHeight; + + push(name, time, null, INTERVAL_START, i, startData); + openWindows.add(windowID); } + + thread.markers = newMarkers; } profile.meta.markerSchema = profile.meta.markerSchema.filter( diff --git a/src/profile-logic/tracks.ts b/src/profile-logic/tracks.ts index e78533ef31..563a01c9e8 100644 --- a/src/profile-logic/tracks.ts +++ b/src/profile-logic/tracks.ts @@ -2,7 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import type { - ScreenshotPayload, Profile, RawProfileSharedData, RawThread, @@ -29,9 +28,9 @@ import { getMarkerTypesForDisplay, } from './marker-data'; import { intersectSets, subtractSets } from '../utils/set'; -import { StringTable } from '../utils/string-table'; import { splitSearchString, stringsToRegExp } from '../utils/string'; import { ensureExists, assertExhaustiveCheck } from '../utils/types'; +import { INTERVAL_END } from '../app-logic/constants'; export type TracksWithOrder = { readonly globalTracks: GlobalTrack[]; @@ -717,12 +716,6 @@ export function computeGlobalTracks( let globalTracks: GlobalTrack[] = []; // Create the global tracks. - const { stringArray } = profile.shared; - const stringTable = StringTable.withBackingArray(stringArray); - const screenshotNameIndex = stringTable.hasString('CompositorScreenshot') - ? stringTable.indexForString('CompositorScreenshot') - : null; - for ( let threadIndex = 0; threadIndex < profile.threads.length; @@ -762,20 +755,23 @@ export function computeGlobalTracks( } } - // Check for screenshots. + // Windows must keep being added in the order their first screenshot was taken: + // shared URLs refer to global tracks by index, + // so a different order would change what an existing URL selects. const ids: Set = new Set(); - if (screenshotNameIndex !== null) { - for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { - if (markers.name[markerIndex] === screenshotNameIndex) { - // Coerce the payload to a screenshot one. Don't do a runtime check that - // this is correct. - const data = markers.data[markerIndex] as ScreenshotPayload; - ids.add(data.windowID); - } - } - for (const id of ids) { - globalTracks.push({ type: 'screenshots', id, threadIndex }); + for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { + const data = markers.data[markerIndex]; + if ( + markers.phase[markerIndex] === INTERVAL_END || + data === null || + data.type !== 'CompositorScreenshot' + ) { + continue; } + ids.add(data.windowID); + } + for (const id of ids) { + globalTracks.push({ type: 'screenshots', id, threadIndex }); } } diff --git a/src/test/components/TooltipMarker.test.tsx b/src/test/components/TooltipMarker.test.tsx index e18af68e04..b8dbd6a589 100644 --- a/src/test/components/TooltipMarker.test.tsx +++ b/src/test/components/TooltipMarker.test.tsx @@ -1205,22 +1205,6 @@ describe('TooltipMarker', function () { expect(container.firstChild).toMatchSnapshot(); }); - it('renders the tooltip of CompositorScreenshotWindowDestroyed', () => { - setupWithPayload([ - [ - 'CompositorScreenshotWindowDestroyed', - 1, - 2, - { - type: 'CompositorScreenshot', - windowID: 'XXX', - }, - ], - ]); - - expect(document.body).toMatchSnapshot(); - }); - it('shows the source thread for markers from a merged thread', function () { // We construct a profile that has 2 threads from 2 different tabs. const tab1Domain = 'https://mozilla.org'; diff --git a/src/test/components/TrackContextMenu.test.tsx b/src/test/components/TrackContextMenu.test.tsx index 89807f1bdc..813a5bd347 100644 --- a/src/test/components/TrackContextMenu.test.tsx +++ b/src/test/components/TrackContextMenu.test.tsx @@ -36,7 +36,7 @@ import { getScreenshotTrackProfile, getNetworkTrackProfile, addIPCMarkerPairToThreads, - getThreadWithMarkers, + getThreadWithRawMarkers, getScreenshotMarkersForWindowId, } from '../fixtures/profiles/processed-profile'; @@ -1229,14 +1229,14 @@ describe('timeline/TrackContextMenu', function () { // add a couple of global screenshots tracks profile.threads.push({ - ...getThreadWithMarkers( + ...getThreadWithRawMarkers( profile.shared, getScreenshotMarkersForWindowId('0', 5) ), tid: profile.threads.length, }); profile.threads.push({ - ...getThreadWithMarkers( + ...getThreadWithRawMarkers( profile.shared, getScreenshotMarkersForWindowId('1', 5) ), diff --git a/src/test/components/TrackScreenshots.test.tsx b/src/test/components/TrackScreenshots.test.tsx index 43ccf90be8..6fbfff7b06 100644 --- a/src/test/components/TrackScreenshots.test.tsx +++ b/src/test/components/TrackScreenshots.test.tsx @@ -204,8 +204,10 @@ describe('timeline/TrackScreenshots', function () { const profile = getScreenshotTrackProfile(); const { shared, threads } = profile; const [thread] = threads; - const markerIndexA = thread.markers.length - 3; - const markerIndexB = thread.markers.length - 2; + // Screenshots are stored as start / end pairs, so these are the start markers + // of the second to last and third to last screenshots. + const markerIndexA = thread.markers.length - 5; + const markerIndexB = thread.markers.length - 3; // We keep the last marker so that the profile's root range is correct. _setScreenshotMarkersToUnknown(thread, shared, markerIndexA, markerIndexB); @@ -232,8 +234,9 @@ describe('timeline/TrackScreenshots', function () { const { shared, threads } = profile; const [thread] = threads; + // The start markers of the first two screenshots. const markerIndexA = 0; - const markerIndexB = 1; + const markerIndexB = 2; _setScreenshotMarkersToUnknown(thread, shared, markerIndexA, markerIndexB); @@ -396,15 +399,11 @@ function _setScreenshotMarkersToUnknown( shared: RawProfileSharedData, ...markerIndexes: IndexIntoRawMarkerTable[] ) { - // Remove off the last few screenshot markers const stringTable = StringTable.withBackingArray(shared.stringArray); const unknownStringIndex = stringTable.indexForString('Unknown'); - const screenshotStringIndex = stringTable.indexForString( - 'CompositorScreenshot' - ); for (const markerIndex of markerIndexes) { // Double check that we've actually got screenshot markers: - if (thread.markers.name[markerIndex] !== screenshotStringIndex) { + if (thread.markers.data[markerIndex]?.type !== 'CompositorScreenshot') { throw new Error('This is not a screenshot marker.'); } thread.markers.name[markerIndex] = unknownStringIndex; diff --git a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap index 19fbb30030..f5fda6d905 100644 --- a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap +++ b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap @@ -1994,74 +1994,6 @@ exports[`TooltipMarker renders properly redirect network markers without additio
`; -exports[`TooltipMarker renders the tooltip of CompositorScreenshotWindowDestroyed 1`] = ` - -
-
-
-
-
- 1ms -
-
- - CompositorScreenshotWindowDestroyed - -
-
-
-
-
- Description - : -
-
- This marker spans the time between each composite of a window and shows the window contents during that time. -
-
- Window ID - : -
- XXX -
- Track - : -
- Empty -
-
-
- -`; - exports[`TooltipMarker renders tooltips for various markers: Bailout_ShapeGuard after getelem on line 3666 of resource://foo.js -> resource://bar.js:3662-10 1`] = `
- 2ms + 1ms
@@ -24,7 +24,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` >
@@ -34,7 +34,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` >
@@ -44,7 +44,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -54,7 +54,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -64,7 +64,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -74,7 +74,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -84,7 +84,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -94,7 +94,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -104,7 +104,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -114,7 +114,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > diff --git a/src/test/fixtures/profiles/processed-profile.ts b/src/test/fixtures/profiles/processed-profile.ts index 6e24047412..924a7f03e8 100644 --- a/src/test/fixtures/profiles/processed-profile.ts +++ b/src/test/fixtures/profiles/processed-profile.ts @@ -372,19 +372,33 @@ export function makeIntervalMarker( * A utility to make TestDefinedRawMarker */ export function makeCompositorScreenshot( - startTime: Milliseconds + startTime: Milliseconds, + windowID: string = '0' ): TestDefinedRawMarker { return { - ...makeInstantMarker('CompositorScreenshot', startTime), + ...makeStartMarker(`CompositorScreenshot ${windowID}`, startTime), data: { type: 'CompositorScreenshot', url: 0, - windowID: '', - windowSize: { width: 100, height: 100 }, + windowID, + windowSize: { width: 300, height: 150 }, }, }; } +/** + * A utility to make TestDefinedRawMarker + */ +export function makeCompositorScreenshotEnd( + endTime: Milliseconds, + windowID: string = '0' +): TestDefinedRawMarker { + return { + ...makeEndMarker(`CompositorScreenshot ${windowID}`, endTime), + data: { type: 'CompositorScreenshot', windowID }, + }; +} + export function getUserTiming( name: string, startTime: Milliseconds, @@ -424,6 +438,20 @@ export function getProfileWithMarkers( return profile; } +export function getProfileWithRawMarkers( + ...markersPerThread: TestDefinedRawMarker[][] +): Profile { + const profile = getEmptyProfile(); + profile.meta.markerSchema = markerSchemaForTests; + + profile.threads = markersPerThread.map((markers, i) => ({ + ...getThreadWithRawMarkers(profile.shared, markers), + tid: i, + })); + addCompositorScreenshotSchemaIfNeeded(profile); + return profile; +} + export const compositorScreenshotMarkerSchema: MarkerSchema = { name: 'CompositorScreenshot', display: ['marker-chart', 'marker-table'], @@ -1408,39 +1436,34 @@ export function getIPCTrackProfile() { return getProfileWithMarkers(arrayOfIPCMarkers); } +/** + * One screenshot per millisecond, starting at 0. + * Each one ends where the next one starts; + * the last one is left open unless `destroyTime` is given. + */ export function getScreenshotMarkersForWindowId( windowID: string, - count: number -): TestDefinedMarker[] { - return Array(count) - .fill(undefined) - .map((_, i) => [ - 'CompositorScreenshot', - i, - null, - { - type: 'CompositorScreenshot', - url: 0, // Some arbitrary string. - windowID, - windowSize: { width: 300, height: 150 }, - }, - ]); + count: number, + destroyTime: Milliseconds | null = null +): TestDefinedRawMarker[] { + const markers: TestDefinedRawMarker[] = []; + for (let i = 0; i < count; i++) { + if (i > 0) { + markers.push(makeCompositorScreenshotEnd(i, windowID)); + } + markers.push(makeCompositorScreenshot(i, windowID)); + } + if (destroyTime !== null) { + markers.push(makeCompositorScreenshotEnd(destroyTime, windowID)); + } + return markers; } export function getScreenshotTrackProfile() { - return getProfileWithMarkers([ + return getProfileWithRawMarkers([ ...getScreenshotMarkersForWindowId('0', 5), // This window isn't closed, so we should repeat the last screenshot - ...getScreenshotMarkersForWindowId('1', 5), // This window is closed after screenshot 6. + ...getScreenshotMarkersForWindowId('1', 5, 6), // This window is closed after screenshot 6. ...getScreenshotMarkersForWindowId('2', 10), // This window isn't closed and define the profile length - [ - 'CompositorScreenshotWindowDestroyed', - 6, - null, - { - type: 'CompositorScreenshot', - windowID: '1', - }, - ], ]); } diff --git a/src/test/store/profile-view.test.ts b/src/test/store/profile-view.test.ts index 58863b5142..580a9694f6 100644 --- a/src/test/store/profile-view.test.ts +++ b/src/test/store/profile-view.test.ts @@ -2079,9 +2079,9 @@ describe('actions/ProfileView', function () { const screenshots = screenshotMarkersById.get('0'); expect(screenshots?.length).toEqual(5); for (const screenshot of screenshots ?? []) { - expect(screenshot.name).toEqual('CompositorScreenshot'); + expect(screenshot.name).toEqual('CompositorScreenshot 0'); } - expect(screenshotMarkersById.get('1')?.length).toEqual(6); + expect(screenshotMarkersById.get('1')?.length).toEqual(5); expect(screenshotMarkersById.get('2')?.length).toEqual(10); }); @@ -2090,9 +2090,9 @@ describe('actions/ProfileView', function () { const [{ markers }] = profile.threads; const { dispatch, getState } = storeWithProfile(profile); - // Double check that there are 21 markers in the test data, and commit a + // Double check that there are 38 raw markers in the test data, and commit a // subsection of that range. - expect(markers.length).toBe(21); + expect(markers.length).toBe(38); dispatch(ProfileView.commitRange(3.1, 7.5)); // Get out the markers. diff --git a/src/test/unit/__snapshots__/marker-data.test.ts.snap b/src/test/unit/__snapshots__/marker-data.test.ts.snap index 344a6dbae1..47bf7283f3 100644 --- a/src/test/unit/__snapshots__/marker-data.test.ts.snap +++ b/src/test/unit/__snapshots__/marker-data.test.ts.snap @@ -186,7 +186,8 @@ Array [ }, }, "end": 25, - "name": "CompositorScreenshot", + "incomplete": true, + "name": "CompositorScreenshot 0x136888400", "start": 25, "threadId": null, }, diff --git a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap index f29aea1813..d280780e75 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap @@ -2318,17 +2318,17 @@ Object { "nativeSymbols": 0, "resources": 1, "stacks": 26, - "strings": 82, + "strings": 83, }, "threads": Array [ Object { "isMainThread": true, "jsAllocationCount": 0, - "markerCount": 483, + "markerCount": 512, "markerNamesTop": Array [ "MessageLoop::RunTask (376)", "BrowserCrApplication::sendEvent (74)", - "CompositorScreenshot (30)", + "CompositorScreenshot id (59)", "LatencyInfo.Flow (2)", "TracingStartedInBrowser (1)", ], @@ -2711,17 +2711,17 @@ Object { "nativeSymbols": 0, "resources": 1, "stacks": 26, - "strings": 82, + "strings": 83, }, "threads": Array [ Object { "isMainThread": true, "jsAllocationCount": 0, - "markerCount": 483, + "markerCount": 512, "markerNamesTop": Array [ "MessageLoop::RunTask (376)", "BrowserCrApplication::sendEvent (74)", - "CompositorScreenshot (30)", + "CompositorScreenshot id (59)", "LatencyInfo.Flow (2)", "TracingStartedInBrowser (1)", ], diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 80c48726c1..45c6ea857c 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -11369,6 +11369,7 @@ Object { "firefox-webcontent", "setTimeout handler", "Element.getBoundingClientRect", + "CompositorScreenshot 42", ], }, "threads": Array [ @@ -11387,6 +11388,7 @@ Object { 5, 1, 4, + 4, 7, 4, ], @@ -11450,6 +11452,10 @@ Object { "type": "DOMEvent", }, Object {}, + Object { + "type": "CompositorScreenshot", + "windowID": "42", + }, Object { "type": "CompositorScreenshot", "url": 4, @@ -11494,40 +11500,43 @@ Object { null, 10, 12, + 12, null, 13, - null, + 14, ], - "length": 13, + "length": 14, "name": Array [ 0, 1, 2, 2, - 3, + 20, 1, 5, 6, 6, 7, - 3, + 20, + 20, 8, - 9, + 20, ], "phase": Array [ 0, 2, 2, 3, - 0, + 2, 3, 0, 2, 3, 1, - 0, + 3, + 2, 1, - 0, + 3, ], "startTime": Array [ 0, @@ -11540,9 +11549,10 @@ Object { 9, null, 11, + null, 12, 3, - 14, + null, ], }, "name": "GeckoMain", diff --git a/src/test/unit/marker-data.test.ts b/src/test/unit/marker-data.test.ts index d01bb97fbf..98237a94e8 100644 --- a/src/test/unit/marker-data.test.ts +++ b/src/test/unit/marker-data.test.ts @@ -31,6 +31,7 @@ import { makeIntervalMarker, makeInstantMarker, makeCompositorScreenshot, + makeCompositorScreenshotEnd, makeStartMarker, makeEndMarker, } from '../fixtures/profiles/processed-profile'; @@ -360,17 +361,17 @@ describe('Derive markers from Gecko phase markers', function () { ]); expect(profile.threads[0].markers.data).toEqual( - Array(3).fill( + Array(4).fill( expect.objectContaining({ type: 'CompositorScreenshot', windowID: String(windowID), }) ) ); - expect(markers.slice(0, 2)).toEqual( + expect(markers).toEqual( [1, 2].map((start) => expect.objectContaining({ - name: 'CompositorScreenshot', + name: `CompositorScreenshot ${windowID}`, start, end: start + 1, data: expect.objectContaining({ @@ -383,7 +384,7 @@ describe('Derive markers from Gecko phase markers', function () { } ); - it('has special handling for CompositorScreenshot', function () { + it('turns the instant CompositorScreenshot markers into intervals', function () { const basePayload = { type: 'CompositorScreenshot' as const, url: 16, @@ -408,44 +409,77 @@ describe('Derive markers from Gecko phase markers', function () { const startTimesForWindowA = [2, 5]; const startTimesForWindowB = [3, 6]; - const { markers, getState } = setupWithTestDefinedMarkers([ - { - name: 'CompositorScreenshot', - startTime: startTimesForWindowA[0], - endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowA[0], - }, - { - name: 'CompositorScreenshot', - startTime: startTimesForWindowB[0], - endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowB[0], - }, + const windowBDestroyedTime = 8; + const windowCDestroyedTime = 4; + + const screenshot = ( + startTime: number, + data: ScreenshotPayload + ): TestDefinedGeckoMarker => ({ + name: 'CompositorScreenshot', + startTime, + endTime: null, + phase: INSTANT, + data, + }); + + const { profile, markers, getState } = setupWithTestDefinedMarkers([ + screenshot(startTimesForWindowA[0], payloadsForWindowA[0]), + screenshot(startTimesForWindowB[0], payloadsForWindowB[0]), + screenshot(startTimesForWindowA[1], payloadsForWindowA[1]), + // Window C had no screenshot in the profile. { - name: 'CompositorScreenshot', - startTime: startTimesForWindowA[1], + name: 'CompositorScreenshotWindowDestroyed', + startTime: windowCDestroyedTime, endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowA[1], + phase: INSTANT, + data: { + type: 'CompositorScreenshot', + windowID: '0xCCCCCCCCC', + }, }, + screenshot(startTimesForWindowB[1], payloadsForWindowB[1]), { - name: 'CompositorScreenshot', - startTime: startTimesForWindowB[1], + name: 'CompositorScreenshotWindowDestroyed', + startTime: windowBDestroyedTime, endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowB[1], + phase: INSTANT, + data: { + type: 'CompositorScreenshot', + windowID: payloadsForWindowB[0].windowID, + }, }, ]); + expect(profile.meta.markerSchema).toContainEqual( + expect.objectContaining({ + name: 'CompositorScreenshot', + display: expect.arrayContaining(['marker-chart']), + }) + ); + const threadRange = selectedThreadSelectors.getThreadRange(getState()); expect(markers).toEqual([ - // The two firsts have a duration from the first screenshot to the next in + // Window C was destroyed without any screenshot, so its marker is only + // known to end there. + { + name: 'CompositorScreenshot 0xCCCCCCCCC', + data: { + type: 'CompositorScreenshot', + windowID: '0xCCCCCCCCC', + }, + start: threadRange.start, + end: windowCDestroyedTime, + category: 0, + threadId: null, + incomplete: true, + }, + + // The two next have a duration from the first screenshot to the next in // the same window. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xAAAAAAAAA', data: { ...payloadsForWindowA[0], url: expect.anything(), @@ -456,7 +490,7 @@ describe('Derive markers from Gecko phase markers', function () { threadId: null, }, { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xBBBBBBBBB', data: { ...payloadsForWindowB[0], url: expect.anything(), @@ -467,9 +501,10 @@ describe('Derive markers from Gecko phase markers', function () { threadId: null, }, - // The 2 lasts have a duration until the end of the thread range. + // Window A is still open, so its last screenshot is extended to the end + // of the thread range. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xAAAAAAAAA', data: { ...payloadsForWindowA[1], url: expect.anything(), @@ -478,15 +513,18 @@ describe('Derive markers from Gecko phase markers', function () { end: threadRange.end, category: 0, threadId: null, + incomplete: true, }, + + // Window B was destroyed, so its last screenshot ends there. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xBBBBBBBBB', data: { ...payloadsForWindowB[1], url: expect.anything(), }, start: startTimesForWindowB[1], - end: threadRange.end, + end: windowBDestroyedTime, category: 0, threadId: null, }, @@ -558,7 +596,7 @@ describe('deriveMarkersFromRawMarkerTable', function () { 'tracing:ArbitraryName', 'Network:Load 32: https://github.com/rustwasm/wasm-bindgen/issues/5', 'FileIO:FileIO', - 'CompositorScreenshot:CompositorScreenshot', + 'CompositorScreenshot:CompositorScreenshot 0x136888400', 'PreferenceRead:PreferenceRead', 'Text:RefreshDriverTick', 'NoPayloadUserData:Navigation::Start', @@ -871,9 +909,10 @@ describe('deriveMarkersFromRawMarkerTable', function () { windowID: '0x136888400', windowSize: { width: 1280, height: 1000 }, }, - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0x136888400', start: 25, end: 25, + incomplete: true, }); }); }); @@ -1011,12 +1050,17 @@ describe('filterRawMarkerTableToRange', () => { end: 5.6, markers: [ makeCompositorScreenshot(0), + makeCompositorScreenshotEnd(3), makeCompositorScreenshot(3), + makeCompositorScreenshotEnd(7), makeCompositorScreenshot(7), ], }); - expect(Array.from(rawMarkerTable.startTime)).toEqual([0, 3]); + // Both markers of a screenshot's pair are kept, and the screenshot taken + // after the range is dropped. + expect(Array.from(rawMarkerTable.startTime)).toEqual([0, 0, 3, 0]); + expect(Array.from(rawMarkerTable.endTime)).toEqual([0, 3, 0, 7]); }); it('keeps a screenshot markers happening before the range if there is no other marker', () => { @@ -1029,7 +1073,7 @@ describe('filterRawMarkerTableToRange', () => { makeInstantMarker('EndMarkerOutOfRange', 8), ], }); - expect(processedMarkerNames).toEqual(['CompositorScreenshot']); + expect(processedMarkerNames).toEqual(['CompositorScreenshot 0']); }); it('filters network markers', () => { From c5ea847de7081938cc516623fb58de777153f820 Mon Sep 17 00:00:00 2001 From: fatadel Date: Thu, 24 Sep 2026 16:18:26 +0200 Subject: [PATCH 6/6] Create screenshot tracks from marker schemas Select screenshot markers through the timeline-screenshots display location and group tracks by marker name. Read image fields from their schema so tracks do not depend on CompositorScreenshot payload names. Use the same classification during sanitization and comparison, and keep first-screenshot ordering so shared track indexes remain stable. --- docs-developer/CHANGELOG-formats.md | 2 + src/components/timeline/GlobalTrack.tsx | 7 +- src/components/timeline/TrackScreenshots.tsx | 73 ++++++++++---- src/profile-logic/marker-data.ts | 22 ----- src/profile-logic/merge-compare.ts | 27 ++++-- .../process-screenshot-markers.ts | 2 +- .../processed-profile-versioning.ts | 2 +- src/profile-logic/sanitize.ts | 10 +- src/profile-logic/tracks.ts | 21 +++-- src/selectors/per-thread/markers.ts | 25 ++++- src/test/components/TrackContextMenu.test.tsx | 2 + src/test/components/TrackScreenshots.test.tsx | 94 ++++++++++++++++++- .../fixtures/profiles/processed-profile.ts | 2 +- src/test/store/profile-view.test.ts | 24 +++-- src/test/store/publish.test.ts | 3 + src/test/store/receive-profile.test.ts | 40 +++++++- src/test/store/tracks.test.ts | 52 ++++++++++ .../profile-upgrading.test.ts.snap | 1 + src/test/unit/marker-data.test.ts | 2 +- src/test/unit/sanitize.test.ts | 24 +++++ src/test/unit/tracks-sanitization.test.ts | 10 +- src/types/markers.ts | 2 + src/types/profile-derived.ts | 2 +- 23 files changed, 361 insertions(+), 88 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index e0332a2e96..d73be18b86 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -14,6 +14,8 @@ These markers are now stored as pairs of start and end markers instead of instan Two marker schema field formats were added to describe these markers: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. +A new marker schema display location, `timeline-screenshots`, was added. Markers in this location create Screenshot tracks and are grouped by marker name. Markers for the same window must use the same name, and markers for different windows must use different names. + ### Version 75 The func table (`profile.shared.funcTable`) representation changed, mirroring the v71 frame table change: diff --git a/src/components/timeline/GlobalTrack.tsx b/src/components/timeline/GlobalTrack.tsx index a8046f1368..358c2ab57d 100644 --- a/src/components/timeline/GlobalTrack.tsx +++ b/src/components/timeline/GlobalTrack.tsx @@ -139,9 +139,12 @@ class GlobalTrackComponent extends PureComponent { ); } case 'screenshots': { - const { threadIndex, id } = globalTrack; + const { threadIndex, markerName } = globalTrack; return ( - + ); } case 'visual-progress': { diff --git a/src/components/timeline/TrackScreenshots.tsx b/src/components/timeline/TrackScreenshots.tsx index 282a55015d..6fe595ba97 100644 --- a/src/components/timeline/TrackScreenshots.tsx +++ b/src/components/timeline/TrackScreenshots.tsx @@ -6,6 +6,7 @@ import { PureComponent } from 'react'; import explicitConnect from 'firefox-profiler/utils/connect'; import { getCommittedRange, + getMarkerSchemaByName, getPreviewSelectionIsBeingModified, } from 'firefox-profiler/selectors/profile'; import { getThreadSelectors } from 'firefox-profiler/selectors/per-thread'; @@ -16,10 +17,13 @@ import { import { updatePreviewSelection } from 'firefox-profiler/actions/profile-view'; import { createPortal } from 'react-dom'; import { computeScreenshotSize } from 'firefox-profiler/profile-logic/marker-data'; +import { getSchemaFromMarker } from 'firefox-profiler/profile-logic/marker-schema'; import { FULL_TRACK_SCREENSHOT_HEIGHT } from 'firefox-profiler/app-logic/constants'; import type { - ScreenshotPayload, + IndexIntoStringTable, + MarkerPayload, + MarkerSchemaByName, ThreadIndex, Thread, Marker, @@ -33,10 +37,11 @@ import './TrackScreenshots.css'; type OwnProps = { readonly threadIndex: ThreadIndex; - readonly windowId: string; + readonly markerName: string; }; type StateProps = { readonly thread: Thread; + readonly markerSchemaByName: MarkerSchemaByName; readonly rangeStart: Milliseconds; readonly rangeEnd: Milliseconds; readonly screenshots: Marker[]; @@ -127,6 +132,7 @@ class Screenshots extends PureComponent { const { screenshots, thread, + markerSchemaByName, isMakingPreviewSelection, width, rangeStart, @@ -134,12 +140,12 @@ class Screenshots extends PureComponent { } = this.props; const { pageX, offsetX, containerTop } = this.state; - let payload: ScreenshotPayload | null = null; + let payload: MarkerPayload | null = null; if (offsetX !== null) { const screenshotIndex = this.findScreenshotAtMouse(offsetX); if (screenshotIndex !== null) { - payload = screenshots[screenshotIndex].data as any; + payload = screenshots[screenshotIndex].data; } } @@ -153,6 +159,7 @@ class Screenshots extends PureComponent { > { {payload ? ( ({ mapStateToProps: (state, ownProps) => { - const { threadIndex, windowId } = ownProps; + const { threadIndex, markerName } = ownProps; const selectors = getThreadSelectors(threadIndex); const { start, end } = getCommittedRange(state); return { thread: selectors.getRangeFilteredThread(state), + markerSchemaByName: getMarkerSchemaByName(state), screenshots: - selectors.getRangeFilteredScreenshotsById(state).get(windowId) || + selectors.getRangeFilteredScreenshotsByName(state).get(markerName) || EMPTY_SCREENSHOTS_TRACK, threadName: selectors.getFriendlyThreadName(state), rangeStart: start, @@ -208,6 +217,7 @@ export const TimelineTrackScreenshots = explicitConnect< type HoverPreviewProps = { readonly thread: Thread; + readonly markerSchemaByName: MarkerSchemaByName; readonly rangeStart: Milliseconds; readonly rangeEnd: Milliseconds; readonly isMakingPreviewSelection: boolean; @@ -216,9 +226,32 @@ type HoverPreviewProps = { readonly containerTop: null | number; readonly width: number; readonly trackHeight: number; - readonly payload: ScreenshotPayload; + readonly payload: MarkerPayload; }; +function getScreenshotImageData( + payload: MarkerPayload | null, + markerSchemaByName: MarkerSchemaByName +): { + url: IndexIntoStringTable; + size: { width: number; height: number }; +} | null { + const schema = getSchemaFromMarker(markerSchemaByName, payload); + if (!payload || !schema) { + return null; + } + for (const { key, format } of schema.fields) { + if (typeof format === 'object' && format.type === 'screenshot-data-url') { + const url = (payload as any)[key] as IndexIntoStringTable | undefined; + const size = (payload as any)[format.sizeFieldForAspectRatio] as + | { width: number; height: number } + | undefined; + return url === undefined || size === undefined ? null : { url, size }; + } + } + return null; +} + const MAXIMUM_HOVER_SIZE = 350; const MAXIMUM_HOVER_SIZE_WHEN_SELECTING_RANGE = 100; @@ -231,6 +264,7 @@ class HoverPreview extends PureComponent { override render() { const { thread, + markerSchemaByName, isMakingPreviewSelection, width, pageX, @@ -244,17 +278,18 @@ class HoverPreview extends PureComponent { return null; } - const { url, windowSize } = payload; - if (url === undefined || windowSize === undefined) { + const imageData = getScreenshotImageData(payload, markerSchemaByName); + if (imageData === null) { return null; } + const { url, size } = imageData; const maximumHoverSize = isMakingPreviewSelection ? MAXIMUM_HOVER_SIZE_WHEN_SELECTING_RANGE : MAXIMUM_HOVER_SIZE; const { width: hoverWidth, height: hoverHeight } = computeScreenshotSize( - windowSize, + size, maximumHoverSize ); @@ -302,6 +337,7 @@ class HoverPreview extends PureComponent { type ScreenshotStripProps = { readonly thread: Thread; + readonly markerSchemaByName: MarkerSchemaByName; readonly rangeStart: Milliseconds; readonly rangeEnd: Milliseconds; readonly screenshots: Marker[]; @@ -313,6 +349,7 @@ class ScreenshotStrip extends PureComponent { override render() { const { thread, + markerSchemaByName, width: outerContainerWidth, rangeStart, rangeEnd, @@ -349,15 +386,15 @@ class ScreenshotStrip extends PureComponent { break; } } - // Coerce the payload into a screenshot one. - const payload: ScreenshotPayload = screenshots[screenshotIndex] - .data as any; - const { url: urlStringIndex, windowSize } = payload; - if (urlStringIndex === undefined || windowSize === undefined) { + const imageData = getScreenshotImageData( + screenshots[screenshotIndex].data, + markerSchemaByName + ); + if (imageData === null) { continue; } - const scaledImageWidth = - (trackHeight * windowSize.width) / windowSize.height; + const { url, size } = imageData; + const scaledImageWidth = (trackHeight * size.width) / size.height; images.push(
{ {/* The following image is centered and cropped by the outer container. */} Marker, - markerIndexes: MarkerIndex[] -): Map { - const idToScreenshotMarkers = new Map(); - for (const markerIndex of markerIndexes) { - const marker = getMarker(markerIndex); - const { data } = marker; - if (data && data.type === 'CompositorScreenshot') { - let markers = idToScreenshotMarkers.get(data.windowID); - if (markers === undefined) { - markers = []; - idToScreenshotMarkers.set(data.windowID, markers); - } - - markers.push(marker); - } - } - - return idToScreenshotMarkers; -} - function _shouldSanitizePIICategory( category: MarkerSchemaPIICategory, PIIToBeRemoved: RemoveProfileInformation diff --git a/src/profile-logic/merge-compare.ts b/src/profile-logic/merge-compare.ts index c13f550acd..edbc9fc4da 100644 --- a/src/profile-logic/merge-compare.ts +++ b/src/profile-logic/merge-compare.ts @@ -37,6 +37,7 @@ import { filterRawMarkerTableToRange, deriveMarkersFromRawMarkerTable, correlateIPCMarkers, + getMarkerTypesForDisplay, } from './marker-data'; import { computeStringIndexMarkerFieldsByDataType } from './marker-schema'; import { ensureExists, getFirstItemFromSet } from '../utils/types'; @@ -147,6 +148,10 @@ export function mergeProfilesForDiffing( // Precompute marker fields that need adjusting. const stringIndexMarkerFieldsByDataType = computeStringIndexMarkerFieldsByDataType(resultProfile.meta.markerSchema); + const screenshotMarkerTypes = getMarkerTypesForDisplay( + resultProfile.meta.markerSchema, + 'timeline-screenshots' + ); // If all profiles have an unknown symbolication status, we keep this unknown // status for the combined profile. Otherwise, we mark the combined profile @@ -202,6 +207,14 @@ export function mergeProfilesForDiffing( } let thread = { ...profile.threads[selectedThreadIndex] }; + // Make sure that screenshot markers make it into the merged profile, even + // if they're not on the selected thread. + thread.markers = getThreadMarkersAndScreenshotMarkers( + profile.threads, + profile.threads[selectedThreadIndex], + screenshotMarkerTypes + ); + transformStacks[i] = translateTransformStack( profileSpecific.transforms[selectedThreadIndex] ?? [], translationMaps @@ -225,13 +238,6 @@ export function mergeProfilesForDiffing( _mapNullableStack(stackIndex, oldStackToNewStackPlusOne) ); - // Make sure that screenshot markers make it into the merged profile, even - // if they're not on the selected thread. - thread.markers = getThreadMarkersAndScreenshotMarkers( - profile.threads, - thread - ); - // We filter the profile using the range from the state for this profile. const zeroAt = getTimeRangeIncludingAllThreads(profile).start; const committedRange = @@ -1479,13 +1485,14 @@ function mergeMarkers(threads: RawThread[]): RawMarkerTable { /** * Returns a RawMarkerTable which contains all the markers from targetThread, - * as well as any CompositorScreenshot markers found on any other threads. + * as well as any screenshot markers found on any other threads. * * `targetThread` is expected to be one of the threads in `threads`. */ function getThreadMarkersAndScreenshotMarkers( threads: RawThread[], - targetThread: RawThread + targetThread: RawThread, + screenshotMarkerTypes: Set ): RawMarkerTable { const targetMarkerTable = getRawMarkerTableBuilderFromExisting( targetThread.markers @@ -1503,7 +1510,7 @@ function getThreadMarkersAndScreenshotMarkers( for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { const data = markers.data[markerIndex]; - if (data === null || data.type !== 'CompositorScreenshot') { + if (data === null || !screenshotMarkerTypes.has(data.type)) { continue; } targetMarkerTable.data.push(data); diff --git a/src/profile-logic/process-screenshot-markers.ts b/src/profile-logic/process-screenshot-markers.ts index 311751e26d..952075e722 100644 --- a/src/profile-logic/process-screenshot-markers.ts +++ b/src/profile-logic/process-screenshot-markers.ts @@ -21,7 +21,7 @@ import type { export const compositorScreenshotMarkerSchema: MarkerSchema = { name: 'CompositorScreenshot', - display: ['marker-chart', 'marker-table'], + display: ['marker-chart', 'marker-table', 'timeline-screenshots'], fields: [ { key: 'url', diff --git a/src/profile-logic/processed-profile-versioning.ts b/src/profile-logic/processed-profile-versioning.ts index 7dd1988d34..91fa18fd34 100644 --- a/src/profile-logic/processed-profile-versioning.ts +++ b/src/profile-logic/processed-profile-versioning.ts @@ -3779,7 +3779,7 @@ const _upgraders: { if (hasCompositorScreenshots) { profile.meta.markerSchema.push({ name: 'CompositorScreenshot', - display: ['marker-chart', 'marker-table'], + display: ['marker-chart', 'marker-table', 'timeline-screenshots'], fields: [ { key: 'url', diff --git a/src/profile-logic/sanitize.ts b/src/profile-logic/sanitize.ts index 614fadfa19..97a143f927 100644 --- a/src/profile-logic/sanitize.ts +++ b/src/profile-logic/sanitize.ts @@ -15,6 +15,7 @@ import { removeURLs } from '../utils/string'; import { filterRawMarkerTableToRangeWithMarkersToDelete, sanitizeMarkerFromSchema, + getMarkerTypesForDisplay, } from './marker-data'; import { getSchemaFromMarker } from './marker-schema'; import { @@ -140,6 +141,11 @@ export function sanitizePII( stringArray, }; + const screenshotMarkerTypes = getMarkerTypesForDisplay( + Object.values(markerSchemaByName), + 'timeline-screenshots' + ); + let stackFlags: Uint8Array | null = null; if (windowIdFromPrivateBrowsing.size > 0) { @@ -344,6 +350,7 @@ export function sanitizePII( PIIToBeRemoved, windowIdFromPrivateBrowsing, markerSchemaByName, + screenshotMarkerTypes, stackFlags ); @@ -459,6 +466,7 @@ function sanitizeThreadPII( PIIToBeRemoved: RemoveProfileInformation, windowIdFromPrivateBrowsing: Set, markerSchemaByName: MarkerSchemaByName, + screenshotMarkerTypes: Set, stackFlags: Uint8Array | null ): RawThread | null { if (PIIToBeRemoved.shouldRemoveThreads.has(threadIndex)) { @@ -517,7 +525,7 @@ function sanitizeThreadPII( if ( PIIToBeRemoved.shouldRemoveThreadsWithScreenshots.has(threadIndex) && currentMarker && - currentMarker.type === 'CompositorScreenshot' + screenshotMarkerTypes.has(currentMarker.type) ) { markersToDelete.add(i); } diff --git a/src/profile-logic/tracks.ts b/src/profile-logic/tracks.ts index 563a01c9e8..c03b11e9f3 100644 --- a/src/profile-logic/tracks.ts +++ b/src/profile-logic/tracks.ts @@ -552,7 +552,7 @@ function _trackIdentityKey( case 'process': return `process:${track.pid}`; case 'screenshots': - return `screenshots:${track.id}`; + return `screenshots:${track.markerName}`; case 'visual-progress': case 'perceptual-visual-progress': case 'contentful-visual-progress': @@ -605,7 +605,7 @@ function _trackIdentityKey( /** * Map each old TrackIndex to its new TrackIndex when both old and new track * lists describe the same profile across a sanitization step. Tracks are - * matched by stable identity (pid, screenshot id, threadIndex, counterIndex, + * matched by stable identity (pid, screenshot marker name, threadIndex, counterIndex, * visual-progress singleton, or marker schema name plus marker name string); * old-side thread, counter, and string-table indexes are normalized through * the supplied translation maps before comparison. Tracks with no match in @@ -714,6 +714,13 @@ export function computeGlobalTracks( }; const globalTracksByPid: Map = new Map(); let globalTracks: GlobalTrack[] = []; + const markerSchema = computeCombinedMarkerSchemaList( + profile.meta.markerSchema || [] + ); + const screenshotTimelineMarkerTypes = getMarkerTypesForDisplay( + markerSchema, + 'timeline-screenshots' + ); // Create the global tracks. for ( @@ -758,20 +765,20 @@ export function computeGlobalTracks( // Windows must keep being added in the order their first screenshot was taken: // shared URLs refer to global tracks by index, // so a different order would change what an existing URL selects. - const ids: Set = new Set(); + const markerNames: Set = new Set(); for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { const data = markers.data[markerIndex]; if ( markers.phase[markerIndex] === INTERVAL_END || data === null || - data.type !== 'CompositorScreenshot' + !screenshotTimelineMarkerTypes.has(data.type) ) { continue; } - ids.add(data.windowID); + markerNames.add(profile.shared.stringArray[markers.name[markerIndex]]); } - for (const id of ids) { - globalTracks.push({ type: 'screenshots', id, threadIndex }); + for (const markerName of markerNames) { + globalTracks.push({ type: 'screenshots', markerName, threadIndex }); } } diff --git a/src/selectors/per-thread/markers.ts b/src/selectors/per-thread/markers.ts index 27d5b86483..d80a0d3ec0 100644 --- a/src/selectors/per-thread/markers.ts +++ b/src/selectors/per-thread/markers.ts @@ -550,14 +550,29 @@ export function getMarkerSelectorsPerThread( MarkerTimingLogic.getMarkerTiming ); + const getRangeFilteredScreenshotMarkerIndexes = + getTimelineMarkerIndexesBySchemaLocation('timeline-screenshots'); + /** - * This groups screenshot markers by their window ID. + * This groups screenshot markers by their name. */ - const getRangeFilteredScreenshotsById: Selector> = + const getRangeFilteredScreenshotsByName: Selector> = createSelector( getMarkerGetter, - getCommittedRangeFilteredMarkerIndexes, - MarkerData.groupScreenshotsById + getRangeFilteredScreenshotMarkerIndexes, + (getMarker, markerIndexes) => { + const screenshotsByName = new Map(); + for (const markerIndex of markerIndexes) { + const marker = getMarker(markerIndex); + let screenshots = screenshotsByName.get(marker.name); + if (screenshots === undefined) { + screenshots = []; + screenshotsByName.set(marker.name, screenshots); + } + screenshots.push(marker); + } + return screenshotsByName; + } ); /** @@ -797,7 +812,7 @@ export function getMarkerSelectorsPerThread( getTimelineIPCMarkerIndexes, getTimelineMarkerIndexesBySchemaLocation, getNetworkTrackTiming, - getRangeFilteredScreenshotsById, + getRangeFilteredScreenshotsByName, getSearchFilteredMarkerIndexes, getPreviewFilteredMarkerIndexes, getSelectedMarkerIndex, diff --git a/src/test/components/TrackContextMenu.test.tsx b/src/test/components/TrackContextMenu.test.tsx index 813a5bd347..97f4ec4f34 100644 --- a/src/test/components/TrackContextMenu.test.tsx +++ b/src/test/components/TrackContextMenu.test.tsx @@ -35,6 +35,7 @@ import { import { getScreenshotTrackProfile, getNetworkTrackProfile, + addCompositorScreenshotSchemaIfNeeded, addIPCMarkerPairToThreads, getThreadWithRawMarkers, getScreenshotMarkersForWindowId, @@ -1242,6 +1243,7 @@ describe('timeline/TrackContextMenu', function () { ), tid: profile.threads.length, }); + addCompositorScreenshotSchemaIfNeeded(profile); const { store } = setup(profile); diff --git a/src/test/components/TrackScreenshots.test.tsx b/src/test/components/TrackScreenshots.test.tsx index 6fbfff7b06..7d803912ae 100644 --- a/src/test/components/TrackScreenshots.test.tsx +++ b/src/test/components/TrackScreenshots.test.tsx @@ -79,6 +79,93 @@ describe('timeline/TrackScreenshots', function () { expect(size).toEqual({ width: 350, height: 175 }); }); + it('renders screenshot fields identified by the schema', () => { + const profile = getScreenshotTrackProfile(); + const { markers } = profile.threads[0]; + const firstPayload = markers.data[0]; + if (firstPayload?.type !== 'CompositorScreenshot') { + throw new Error('Expected a screenshot marker.'); + } + const imageUrl = profile.shared.stringArray[ensureExists(firstPayload.url)]; + markers.data = markers.data.map((payload) => { + if (payload?.type !== 'CompositorScreenshot') { + return payload; + } + return { + type: 'NoPayloadUserData' as const, + ...(payload.url !== undefined && { image: payload.url }), + ...(payload.windowSize !== undefined && { + dimensions: { width: 100, height: 200 }, + }), + }; + }); + profile.meta.markerSchema = [ + { + name: 'NoPayloadUserData', + display: ['timeline-screenshots'], + fields: [ + { key: 'dimensions', format: 'screenshot-size' }, + { + key: 'image', + format: { + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'dimensions', + }, + }, + ], + }, + ]; + + const { container, moveMouseAndGetImageSize, screenshotHover } = + setup(profile); + const thumbnail = ensureExists( + container.querySelector('.timelineTrackScreenshotImg') + ); + expect(thumbnail).toHaveAttribute('src', imageUrl); + expect(thumbnail).toHaveStyle({ + width: `${FULL_TRACK_SCREENSHOT_HEIGHT / 2}px`, + height: `${FULL_TRACK_SCREENSHOT_HEIGHT}px`, + }); + expect(moveMouseAndGetImageSize(LEFT)).toEqual({ + width: 175, + height: 350, + }); + expect(screenshotHover().querySelector('img')).toHaveAttribute( + 'src', + imageUrl + ); + }); + + it.each(['url', 'windowSize'] as const)( + 'does not render images without %s', + (field) => { + const profile = getScreenshotTrackProfile(); + for (const payload of profile.threads[0].markers.data) { + if (payload?.type === 'CompositorScreenshot') { + delete payload[field]; + } + } + + const { container, moveMouse, screenshotHover } = setup(profile); + expect(container.querySelector('img')).toBeNull(); + moveMouse(LEFT); + expect(screenshotHover).toThrow(); + } + ); + + it('does not render images without a screenshot image field in the schema', () => { + const profile = getScreenshotTrackProfile(); + profile.meta.markerSchema = profile.meta.markerSchema.map((schema) => ({ + ...schema, + fields: [], + })); + + const { container, moveMouse, screenshotHover } = setup(profile); + expect(container.querySelector('img')).toBeNull(); + moveMouse(LEFT); + expect(screenshotHover).toThrow(); + }); + it('sets a preview selection when clicking with the mouse', () => { const { selectionOverlay, screenshotClick, getState } = setup( undefined, @@ -287,7 +374,12 @@ describe('timeline/TrackScreenshots', function () { function setup( profile: Profile = getScreenshotTrackProfile(), - component = + component = ( + + ) ) { const store = storeWithProfile(profile); const { getState, dispatch } = store; diff --git a/src/test/fixtures/profiles/processed-profile.ts b/src/test/fixtures/profiles/processed-profile.ts index 924a7f03e8..8612906e47 100644 --- a/src/test/fixtures/profiles/processed-profile.ts +++ b/src/test/fixtures/profiles/processed-profile.ts @@ -454,7 +454,7 @@ export function getProfileWithRawMarkers( export const compositorScreenshotMarkerSchema: MarkerSchema = { name: 'CompositorScreenshot', - display: ['marker-chart', 'marker-table'], + display: ['marker-chart', 'marker-table', 'timeline-screenshots'], fields: [ { key: 'url', diff --git a/src/test/store/profile-view.test.ts b/src/test/store/profile-view.test.ts index 580a9694f6..25f79496bf 100644 --- a/src/test/store/profile-view.test.ts +++ b/src/test/store/profile-view.test.ts @@ -2067,22 +2067,26 @@ describe('actions/ProfileView', function () { }); }); - describe('getRangeFilteredScreenshotsById', function () { + describe('getRangeFilteredScreenshotsByName', function () { it('can extract some screenshot markers', function () { const profile = getScreenshotTrackProfile(); const { getState } = storeWithProfile(profile); - const screenshotMarkersById = - selectedThreadSelectors.getRangeFilteredScreenshotsById(getState()); - const keys = [...screenshotMarkersById.keys()]; + const screenshotMarkersByName = + selectedThreadSelectors.getRangeFilteredScreenshotsByName(getState()); + const keys = [...screenshotMarkersByName.keys()]; expect(keys.length).toEqual(3); - const screenshots = screenshotMarkersById.get('0'); + const screenshots = screenshotMarkersByName.get('CompositorScreenshot 0'); expect(screenshots?.length).toEqual(5); for (const screenshot of screenshots ?? []) { expect(screenshot.name).toEqual('CompositorScreenshot 0'); } - expect(screenshotMarkersById.get('1')?.length).toEqual(5); - expect(screenshotMarkersById.get('2')?.length).toEqual(10); + expect( + screenshotMarkersByName.get('CompositorScreenshot 1')?.length + ).toEqual(5); + expect( + screenshotMarkersByName.get('CompositorScreenshot 2')?.length + ).toEqual(10); }); it('filters screenshots within a range selection', function () { @@ -2096,9 +2100,9 @@ describe('actions/ProfileView', function () { dispatch(ProfileView.commitRange(3.1, 7.5)); // Get out the markers. - const screenshotMarkersById = - selectedThreadSelectors.getRangeFilteredScreenshotsById(getState()); - const screenshots = screenshotMarkersById.get('2'); + const screenshotMarkersByName = + selectedThreadSelectors.getRangeFilteredScreenshotsByName(getState()); + const screenshots = screenshotMarkersByName.get('CompositorScreenshot 2'); if (!screenshots) { throw new Error('No screenshots found.'); } diff --git a/src/test/store/publish.test.ts b/src/test/store/publish.test.ts index a38d10b558..8d9423af1a 100644 --- a/src/test/store/publish.test.ts +++ b/src/test/store/publish.test.ts @@ -44,6 +44,7 @@ import { urlFromState } from '../../app-logic/url-handling'; import { getHasZipFile } from '../../selectors/zipped-profiles'; import { getProfileFromTextSamples, + addCompositorScreenshotSchemaIfNeeded, addRawMarkersToThread, makeCompositorScreenshot, } from '../fixtures/profiles/processed-profile'; @@ -670,6 +671,7 @@ describe('attemptToPublish', function () { addRawMarkersToThread(profile.threads[0], profile.shared, [ makeCompositorScreenshot(0.5), ]); + addCompositorScreenshotSchemaIfNeeded(profile); const store = storeWithProfile(profile); const { dispatch, getState, resolveUpload, assertUploadSuccess } = @@ -771,6 +773,7 @@ describe('attemptToPublish', function () { addRawMarkersToThread(profile.threads[2], profile.shared, [ makeCompositorScreenshot(0.5), ]); + addCompositorScreenshotSchemaIfNeeded(profile); const store = storeWithProfile(profile); const { dispatch, getState, resolveUpload, assertUploadSuccess } = diff --git a/src/test/store/receive-profile.test.ts b/src/test/store/receive-profile.test.ts index a768f16605..39073b85ac 100644 --- a/src/test/store/receive-profile.test.ts +++ b/src/test/store/receive-profile.test.ts @@ -1750,7 +1750,7 @@ describe('actions/receive-profile', function () { //Get profiles with one screenshot track const profile1 = getProfileWithMarkers([ [ - 'CompositorScreenshot', + 'CompositorScreenshot 0', 0, null, { @@ -1763,7 +1763,7 @@ describe('actions/receive-profile', function () { ]); const profile2 = getProfileWithMarkers([ [ - 'CompositorScreenshot', + 'CompositorScreenshot 1', 0, null, { @@ -1795,6 +1795,42 @@ describe('actions/receive-profile', function () { ]); }); + it('includes the screenshot track when only one profile has screenshot markers', async function () { + const store = blankStore(); + const profile1 = getProfileWithMarkers([]); + const profile2 = getProfileWithMarkers([ + [ + 'CompositorScreenshot 1', + 0, + null, + { + type: 'CompositorScreenshot', + url: 0, // Some arbitrary string. + windowID: '1', + windowSize: { width: 300, height: 150 }, + }, + ], + ]); + const { resultProfile } = await setup( + { + profile1: profile1, + profile2: profile2, + }, + { + url1: 'https://fakeurl.com/public/fakehash1/?thread=0&v=3', + url2: 'https://fakeurl.com/public/fakehash1/?thread=0&v=3', + } + ); + + store.dispatch(viewProfile(resultProfile)); + expect(getHumanReadableTracks(store.getState())).toEqual([ + 'show [screenshots]', + 'show [thread Empty default] SELECTED', + 'show [thread Empty default]', + 'show [thread Diff between 1 and 2 comparison]', + ]); + }); + it('gives a fatal error when the selected thread index is out of bounds', async function () { const { dispatch, getState } = blankStore(); const { profile1, profile2 } = getSomeProfiles(); diff --git a/src/test/store/tracks.test.ts b/src/test/store/tracks.test.ts index 4a128f35be..8361c2e294 100644 --- a/src/test/store/tracks.test.ts +++ b/src/test/store/tracks.test.ts @@ -7,8 +7,11 @@ import { getNetworkTrackProfile, addIPCMarkerPairToThreads, getProfileWithMarkers, + getProfileWithRawMarkers, getProfileFromTextSamples, getProfileWithThreadCPUDelta, + makeCompositorScreenshot, + makeCompositorScreenshotEnd, } from '../fixtures/profiles/processed-profile'; import { getEmptyThread } from '../../profile-logic/data-structures'; import { storeWithProfile } from '../fixtures/stores'; @@ -345,6 +348,55 @@ describe('ordering and hiding', function () { ]); }); + it('keeps screenshot track indexes stable when there are unmatched end markers', function () { + const profile = getProfileWithRawMarkers([ + makeCompositorScreenshotEnd(0, '0'), + makeCompositorScreenshotEnd(1, '2'), + makeCompositorScreenshot(2, '1'), + makeCompositorScreenshot(3, '2'), + makeCompositorScreenshotEnd(4, '1'), + makeCompositorScreenshotEnd(5, '2'), + ]); + const { getState } = storeWithProfile(profile); + + expect(ProfileViewSelectors.getGlobalTracks(getState())).toEqual([ + expect.objectContaining({ type: 'process' }), + { + type: 'screenshots', + markerName: 'CompositorScreenshot 1', + threadIndex: 0, + }, + { + type: 'screenshots', + markerName: 'CompositorScreenshot 2', + threadIndex: 0, + }, + ]); + }); + + it('creates screenshot tracks from the schema display location', function () { + const customScreenshot = { + type: 'Url' as const, + url: '', + windowID: 42, + }; + const profile = getProfileWithMarkers([ + ['CustomScreenshot', 0, null, customScreenshot], + ]); + profile.meta.markerSchema.push({ + name: 'Url', + display: ['marker-chart', 'timeline-screenshots'], + fields: [], + }); + + const { getState } = storeWithProfile(profile); + expect(ProfileViewSelectors.getGlobalTracks(getState())).toContainEqual({ + type: 'screenshots', + markerName: 'CustomScreenshot', + threadIndex: 0, + }); + }); + describe('sorting of track types to ensure proper URL backwards compatibility', function () { const stableIndexOrder = [ 'process', diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 45c6ea857c..f810c99811 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -10797,6 +10797,7 @@ Object { "display": Array [ "marker-chart", "marker-table", + "timeline-screenshots", ], "fields": Array [ Object { diff --git a/src/test/unit/marker-data.test.ts b/src/test/unit/marker-data.test.ts index 98237a94e8..fc73025304 100644 --- a/src/test/unit/marker-data.test.ts +++ b/src/test/unit/marker-data.test.ts @@ -454,7 +454,7 @@ describe('Derive markers from Gecko phase markers', function () { expect(profile.meta.markerSchema).toContainEqual( expect.objectContaining({ name: 'CompositorScreenshot', - display: expect.arrayContaining(['marker-chart']), + display: expect.arrayContaining(['timeline-screenshots']), }) ); diff --git a/src/test/unit/sanitize.test.ts b/src/test/unit/sanitize.test.ts index 18fcf0b560..5a00a8aa3e 100644 --- a/src/test/unit/sanitize.test.ts +++ b/src/test/unit/sanitize.test.ts @@ -13,6 +13,7 @@ import { addMarkersToThreadWithCorrespondingSamples, addInnerWindowIdToStacks, getNetworkMarkers, + compositorScreenshotMarkerSchema, } from '../fixtures/profiles/processed-profile'; import { ensureExists } from '../../utils/types'; import { @@ -162,6 +163,7 @@ describe('sanitizePII', function () { }, ], }, + CompositorScreenshot: compositorScreenshotMarkerSchema, ...extraMarkerSchemas, }; const markerSchemaByName = computeMarkerSchemaByName( @@ -578,6 +580,28 @@ describe('sanitizePII', function () { } }); + it('should sanitize the screenshots of any marker type shown in a screenshot track', function () { + const profile = getProfileWithMarkers([ + ['MyScreenshot', 0, 1, { type: 'MyScreenshot', url: 0 } as any], + ['Other', 0, 1], + ]); + const { sanitizedProfile } = setup( + { shouldRemoveThreadsWithScreenshots: new Set([0]) }, + profile, + { + MyScreenshot: { + name: 'MyScreenshot', + display: ['marker-chart', 'timeline-screenshots'], + fields: [], + }, + } + ); + + const { markers } = sanitizedProfile.threads[0]; + expect(markers.length).toBe(1); + expect(sanitizedProfile.shared.stringArray[markers.name[0]]).toBe('Other'); + }); + it('should sanitize the pages information', function () { const { originalProfile, sanitizedProfile } = setup({ shouldRemoveUrls: true, diff --git a/src/test/unit/tracks-sanitization.test.ts b/src/test/unit/tracks-sanitization.test.ts index 6880d76052..00cd5d25d7 100644 --- a/src/test/unit/tracks-sanitization.test.ts +++ b/src/test/unit/tracks-sanitization.test.ts @@ -33,13 +33,13 @@ describe('computeOldTrackIndexToNewTrackIndexMap', function () { expect(map.get(2)).toBe(1); }); - it('matches screenshot tracks by id', function () { + it('matches screenshot tracks by marker name', function () { const oldTracks: Track[] = [ - { type: 'screenshots', id: 'win-A', threadIndex: 0 }, - { type: 'screenshots', id: 'win-B', threadIndex: 0 }, + { type: 'screenshots', markerName: 'win-A', threadIndex: 0 }, + { type: 'screenshots', markerName: 'win-B', threadIndex: 0 }, ]; const newTracks: Track[] = [ - { type: 'screenshots', id: 'win-B', threadIndex: 0 }, + { type: 'screenshots', markerName: 'win-B', threadIndex: 0 }, ]; const map = computeOldTrackIndexToNewTrackIndexMap({ oldTracks, @@ -234,7 +234,7 @@ describe('computeOldTrackIndexToNewTrackIndexMap', function () { it('handles a mixed track list (process + screenshots + visual-progress)', function () { const oldTracks: Track[] = [ { type: 'process', pid: '1', mainThreadIndex: 0 }, - { type: 'screenshots', id: 'win-A', threadIndex: 0 }, + { type: 'screenshots', markerName: 'win-A', threadIndex: 0 }, { type: 'process', pid: '2', mainThreadIndex: 1 }, { type: 'visual-progress' }, ]; diff --git a/src/types/markers.ts b/src/types/markers.ts index 5eb6cd9fd4..3563841668 100644 --- a/src/types/markers.ts +++ b/src/types/markers.ts @@ -125,6 +125,8 @@ export type MarkerDisplayLocation = | 'timeline-fileio' // This adds markers to the Network track. | 'timeline-network' + // This adds markers to the Screenshots track. + | 'timeline-screenshots' // TODO - This is not supported yet. | 'stack-chart'; diff --git a/src/types/profile-derived.ts b/src/types/profile-derived.ts index 682c845df5..10fbae4239 100644 --- a/src/types/profile-derived.ts +++ b/src/types/profile-derived.ts @@ -733,7 +733,7 @@ export type GlobalTrack = } | { readonly type: 'screenshots'; - readonly id: string; + readonly markerName: string; readonly threadIndex: ThreadIndex; } | { readonly type: 'visual-progress' }