1262134 - layers::Image::GetSerial() wraps around and lacks a definition of valid serial numbers

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, enhancement, P3)

Description

[Andreas Pehrson [:pehrsons]]

Image serials are int32_t integers that start at 1 (`++Atomic<int32_t>()`) and will eventually wrap around (e.g., after 400 days of 60fps video as noted by jesup).

In WebRTC (MediaPipeline.cpp) we use -1 to denote an invalid Image serial number. The question then arose whether that was a valid assumption or not. Looking at the code it isn't, although impact is low for us (after it has wrapped to -1 we risk skipping that frame if the previous frame was disabled in its MediaStreamTrack).

The point of this bug is to get clarification on usage of the serial number, preferably as a code comment on Image::GetSerial, so we can adjust WebRTC code accordingly.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

There are some (unusual) usecases where one might reach wraparound (mostly around cases with high framerates and long streams, like security/continuous-streaming applications).  But not high or even medium priority.  Related is the question of why it's signed to start with.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Iirc signed was used so that -1 could be used a sentinel, and 32bit was used to save memory. Neither of these seem particularly like convincing arguments, so feel free to make changes here.

Comment 3

[Nicolas Silva [:nical]]

Most layers serials are unsigned, start with 1 and use 0 as the invalid id (although most of them don't check when they wrap around back to 0, which the should). Perhaps this could be solved with some helper classes like Serial32Generator/AtomicSerial32Generator (and 64bits variants) with a method like GenSerial() that avoid returning 0 when wrapping around.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Generic Serial/SerialGenerator helpers

I was thinking something like this, could be moved to mfbt if there's any interest.
I'd happily replace all of the serials in gfx/layers by this.

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8738541 [details] [diff] [review]
Generic Serial/SerialGenerator helpers

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

LGTM!

::: gfx/layers/Serial.h
@@ +10,5 @@
> +template<typename T>
> +struct Serial {
> +
> +  // Creates an invalid serial by default
> +  Serial(T aHanle = 0) : mHandle(aHanle) {}

s/aHanle/aHandle/

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Nicolas, do we still care about that stuff or it should be wontfix?

Comment 7

[Nicolas Silva [:nical]]

I don't know if we care a lot about the original problem described in the bug. Let's wontfix and reopen if need be.