Geckoprofiler visualProgress timestamp is null

[soulgalore]

In the Geckoprofiler.json the visual metrics progress field is broken as reported in firefox-devtools/profiler#3966 (comment)

"visualMetrics":{"FirstVisualChange":766,"LastVisualChange":2200,"SpeedIndex":832,"VisualProgress":[{"timestamp":null,"percent":0},{"timestamp":null,"percent":1},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100},{"timestamp":null,"percent":100}],"videoRecordingStart":3900,"VisualReadiness":1434,"VisualComplete85":833,"VisualComplete95":833,"VisualComplete99":833}}

There are some massaging of the data that seems broken in:
https://github.com/sitespeedio/browsertime/blob/main/lib/firefox/webdriver/firefox.js#L193-L230

[soulgalore]

Looking at the code (https://github.com/sitespeedio/browsertime/blob/main/lib/firefox/webdriver/firefox.js#L210-L213) that seems to be something that comes from the FirefoxWindowRecorder. That extra calculation should probably not be used when you use ffmpeg recorded video.

[julienw]

I'm surprised that adjustVisualProgressTimestamps doesn't give NaN values instead of undefined...

[soulgalore]

Including @gmierz: what do you think is the best way to get when in time the recording actually starts with ffmpeg (like firstFrameStartTime with window recorder)?

[gmierz]

@soulgalore assuming you mean when the page starts loading, that would be the first frame after the last orange frame. Note that I was poking around at the recordingStartTime metric and I think it might be causing issues:

browsertime/lib/video/screenRecording/firefox/firefoxWindowRecorder.js

Lines 60 to 64 in bcdaeae

	function findRecordingStartTime(recordingDir) {
	// Recording directory has the following format:
	// .../browsertime-results/<web address>/<date time>/windowrecording-<unix recording start time>
	return recordingDir.substr(recordingDir.lastIndexOf('-') + 1);
	}

It's unclear to me why this time needs to be a timestamp. However, if you absolutely need a UNIX timestamp here, I would check the creation time of the MP4 for a timestamp, and then modify it from there. I've noticed that I get folders without the timestamps that it's searching for.

[soulgalore]

I don't need it, someone at Mozilla once needed it I guess? :D For me we could remove this but if you feel it adds value we should aim to fix it.

[mstange]

It's unclear to me why this time needs to be a timestamp.

The Firefox window recorder code contains the following comment which justifies the use of a timestamp:

https://searchfox.org/mozilla-central/rev/1c6bb1476e86b6d428b26d04e6ea3f4367528c77/gfx/layers/CompositionRecorder.cpp#39-44

// The directory has the format of
// "[LayersWindowRecordingPath]/windowrecording-[mRecordingStartTime as unix
// timestamp]". We need mRecordingStart as a unix timestamp here because we
// want the consumer of these files to be able to compute an absolute
// timestamp of each screenshot, so that we can align screenshots with timed
// data from other sources, such as Gecko profiler information.

[mstange]

And @brennie says: "I believe it needs a timestamp because all the frames we write have timestamps in them and we otherwise dont have the timestamp of when the recording started"