Opengl timers broken after f2298f3

[AirPort]

mpv version and platform

git master
macOS 10.12.6

Reproduction steps

Open a video.

Expected behavior

Timers work.

Actual behavior

They're all stuck to 0. The culprit seems to be commit f2298f3, before that they work fine.

Log file

Log with --msg-level=vo=debug: https://0x0.st/dXC.txt

[haasn]

The new method of querying timers is not supported on OS X. So this is expected behavior, unfortunately.

(Going back to the old is not really easily possible due to the change in API and usage)

[Argon-]

Recently it feels like OSX supported in mpv actually decreases. Well, "feels" is the wrong word, it's not a feeling.

[Hrxn]

The problem is OpenGL support on macOS, and their new API, Metal.

Time for Vulkan, I guess....

[Argon-]

No it's not because it used to work.
That's all a user sees: something used to work and now it doesn't work anymore. And regarding features, that's all what matters in the end (to me as well).

[bjin]

@haasn Could you elaborate a bit how difficult it was to use the old method? Also, what's the new requirement (e.g. GL extension) for the new method? It's not immediately clear from commit f2298f3 alone.

[Argon-]

Just googled that myself a few minutes ago. Seems like GL_TIMESTAMP returns 0 on OSX, GL_TIME_ELAPSED works.
When I googled this, I found these statements from some people on an Apple(?) mailinglist where a user asked about this problem. However, I do not know how accurate/true they are.

As others have said: TIMESTAMP is meaningless. The spec simply doesn't allow for the API to provide you enough information to interpret the results. D3D lets you discover whether timestamps are discontiguous (a clock rate change, or an integer overflow). OpenGL does not; nor do the GPUs' internal clocks wrap at powers of two, which might be possible for an API client to adjust for. The problem is not fixable without additional API, and in my opinion, it's better to report 0 query bits than return completely unreliable numbers which might, under some circumstances, appear to be meaningful. Additionally, the glGetIntegerv(GL_TIMESTAMP, ...) is under-specified and problematic for other reasons. The entire spec is a complete bust.

Note that GL doesn't have the notion of disjoint timers, unlike D3D1x, so even if GL_TIMESTAMP were working, you'd probably get somewhat unreliable results, as the GPU clock can be variable depending on power load. That said, 'mostly correct' would be far preferable to me than 'nothing'.

https://lists.apple.com/archives/mac-opengl/2014/Nov/msg00005.html

[haasn]

Recently it feels like OSX supported in mpv actually decreases. Well, "feels" is the wrong word, it's not a feeling.

Well Apple just completely stopped caring about making sure OpenGL works on their platform. This is different from the past, before Metal existed. mpv has always been getting new features, it's just that the new features it's getting now break on OS X OpenGL because OS X OpenGL stopped getting updated.

Could you elaborate a bit how difficult it was to use the old method?

GL_TIME_ELAPSED is not re-entrant, which is what caused e.g. the wrong measurements for stuff like OSD passes (which the new code solves). It's also further away from the vulkan API, which is part of the reason the API was changed. (RA APIs are closer to vulkan than to OpenGL to facilitate porting opengl/video.c to vulkan)

Also, what's the new requirement (e.g. GL extension) for the new method?

The problem is, there's no extension. It's technically supported, they just don't return any measurements. (Timestamp precision is 0)

[ghost]

Hm I think Apple is complaining about how you can't discover timestamps overflow with GL_TIMESTAMP? Not sure if I follow.

[sosia]

Well Apple just completely stopped caring about making sure OpenGL works on their platform. This is different from the past, before Metal existed. mpv has always been getting new features, it's just that the new features it's getting now break on OS X OpenGL because OS X OpenGL stopped getting updated.

That's absolutely true, Apple last updated OpenGL on Os X with Mavericks 4 years ago, and then just stopped caring. And I think everyone agrees that advancement and new features can't be stopped because of one platform's shortcomings.
But it's also true that just in the last month at least three old, non-obscure features that used to work are now busted or simply non-working. And the future doesn't exactly seem bright on this platform.

[haasn]

D3D lets you discover whether timestamps are discontiguous (a clock rate change, or an integer overflow). OpenGL does not; nor do the GPUs' internal clocks wrap at powers of two, which might be possible for an API client to adjust for.

Seems like every other platform compensates for this by actually reporting 64-bit results, which roll over once every 800 years.

Note that GL doesn't have the notion of disjoint timers, unlike D3D1x, so even if GL_TIMESTAMP were working, you'd probably get somewhat unreliable results, as the GPU clock can be variable depending on power load.

This is irrelevant. They should be using wall clock time units, regardless of the GPU frequency.

Sounds to me like they just don't care about doing it properly and would rather return nothing than return something broken (a decision I agree with, at least the second part).

[miosenpai]

The bug appears to also affect Windows builds using Argon's stats Lua script, examples below. Same symptoms occur with local files and default build arguments with no configuration file in use.

[haasn]

@miosenpai What drivers?

[AirPort]

I'm not at all competent with OGL so correct me if I'm wrong, but after @miosenpai report I checked ANGLE's repo and it seems to me that Google devs chose the same approach of Apple ones on this: google/angle@f04f667

[haasn]

This patch could fix it...

diff --git a/video/out/opengl/common.c b/video/out/opengl/common.c
index c7a714817a..f4eb0b9b7a 100644
--- a/video/out/opengl/common.c
+++ b/video/out/opengl/common.c
@@ -286,6 +286,7 @@ static const struct gl_functions gl_functions[] = {
             DEF_FN(EndQuery),
             DEF_FN(QueryCounter),
             DEF_FN(IsQuery),
+            DEF_FN(GetQueryiv),
             DEF_FN(GetQueryObjectiv),
             DEF_FN(GetQueryObjecti64v),
             DEF_FN(GetQueryObjectuiv),
@@ -628,6 +629,14 @@ void mpgl_load_functions2(GL *gl, void *(*get_fn)(void *ctx, const char *n),
     if (gl->DispatchCompute && gl->BindImageTexture)
         gl->mpgl_caps |= MPGL_CAP_COMPUTE_SHADER;
 
+    // GL_ARB_timer_query & high precision timestamp queries
+    if (gl->QueryCounter) {
+        GLint bits;
+        gl->GetQueryiv(GL_TIMESTAMP, GL_QUERY_COUNTER_BITS, &bits);
+        if (bits > 32)
+            gl->mpgl_caps |= MPGL_CAP_TIMESTAMP;
+    }
+
     // Provided for simpler handling if no framebuffer support is available.
     if (!gl->BindFramebuffer)
         gl->BindFramebuffer = &dummy_glBindFramebuffer;
diff --git a/video/out/opengl/common.h b/video/out/opengl/common.h
index 6d8015c8b3..2991cc0bce 100644
--- a/video/out/opengl/common.h
+++ b/video/out/opengl/common.h
@@ -57,6 +57,7 @@ enum {
     MPGL_CAP_SSBO               = (1 << 21),    // GL_ARB_shader_storage_buffer_object
     MPGL_CAP_COMPUTE_SHADER     = (1 << 22),    // GL_ARB_compute_shader & GL_ARB_shader_image_load_store
     MPGL_CAP_NESTED_ARRAY       = (1 << 23),    // GL_ARB_arrays_of_arrays
+    MPGL_CAP_TIMESTAMP          = (1 << 24),    // GL_TIMESTAMP queries supported
 
     MPGL_CAP_SW                 = (1 << 30),    // indirect or sw renderer
 };
@@ -208,6 +209,7 @@ struct GL {
     void (GLAPIENTRY *EndQuery)(GLenum);
     void (GLAPIENTRY *QueryCounter)(GLuint, GLenum);
     GLboolean (GLAPIENTRY *IsQuery)(GLuint);
+    void (GLAPIENTRY *GetQueryiv)(GLenum, GLenum, GLint *);
     void (GLAPIENTRY *GetQueryObjectiv)(GLuint, GLenum, GLint *);
     void (GLAPIENTRY *GetQueryObjecti64v)(GLuint, GLenum, GLint64 *);
     void (GLAPIENTRY *GetQueryObjectuiv)(GLuint, GLenum, GLuint *);
diff --git a/video/out/opengl/gl_headers.h b/video/out/opengl/gl_headers.h
index 9f479dd42f..f36bd47d17 100644
--- a/video/out/opengl/gl_headers.h
+++ b/video/out/opengl/gl_headers.h
@@ -65,6 +65,7 @@
 
 #define GL_TIME_ELAPSED                   0x88BF
 #define GL_TIMESTAMP                      0x8E28
+#define GL_QUERY_COUNTER_BITS             0x8864
 
 // --- GL 4.3 or GL_ARB_debug_output
 
diff --git a/video/out/opengl/ra_gl.c b/video/out/opengl/ra_gl.c
index 6509d9043a..9fcc2bed18 100644
--- a/video/out/opengl/ra_gl.c
+++ b/video/out/opengl/ra_gl.c
@@ -921,6 +921,7 @@ struct gl_timer {
     GLuint stop[GL_QUERY_OBJECT_NUM];
     int idx;
     uint64_t result;
+    bool active;
 };
 
 static ra_timer *gl_timer_create(struct ra *ra)
@@ -952,29 +953,53 @@ static void gl_timer_destroy(struct ra *ra, ra_timer *ratimer)
 
 static void gl_timer_start(struct ra *ra, ra_timer *ratimer)
 {
-    GL *gl = ra_gl_get(ra);
+    struct ra_gl *p = ra->priv;
+    GL *gl = p->gl;
     struct gl_timer *timer = ratimer;
 
+    // GL_TIME_ELAPSED queries are not re-entrant, so just do nothing instead
+    // of crashing. Work-around for platforms that don't support GL_TIMESTAMP
+    if (p->timer_active)
+        return;
+
     // If this query object already contains a result, we need to retrieve it
     timer->result = 0;
     if (gl->IsQuery(timer->start[timer->idx])) {
         uint64_t start = 0, stop = 0;
         gl->GetQueryObjectui64v(timer->start[timer->idx], GL_QUERY_RESULT, &start);
-        gl->GetQueryObjectui64v(timer->stop[timer->idx], GL_QUERY_RESULT, &stop);
-        timer->result = stop - start;
+        if (gl->IsQuery(timer->stop[timer->idx])) {
+            gl->GetQueryObjectui64v(timer->stop[timer->idx], GL_QUERY_RESULT, &stop);
+            timer->result = stop - start;
+        } else {
+            timer->result = start;
+        }
     }
 
-    gl->QueryCounter(timer->start[timer->idx], GL_TIMESTAMP);
+    if (gl->mpgl_caps & MPGL_CAP_TIMESTAMP) {
+        gl->QueryCounter(timer->start[timer->idx], GL_TIMESTAMP);
+    } else {
+        gl->BeginQuery(GL_TIME_ELAPSED, timer->start[timer->idx]);
+        p->timer_active = timer->active = true;
+    }
 }
 
 static uint64_t gl_timer_stop(struct ra *ra, ra_timer *ratimer)
 {
-    GL *gl = ra_gl_get(ra);
+    struct ra_gl *p = ra->priv;
+    GL *gl = p->gl;
     struct gl_timer *timer = ratimer;
 
-    gl->QueryCounter(timer->stop[timer->idx++], GL_TIMESTAMP);
-    timer->idx %= GL_QUERY_OBJECT_NUM;
+    if (p->timer_active && !timer->active)
+        return 0;
+
+    if (timer->active) {
+        gl->EndQuery(GL_TIME_ELAPSED);
+        p->timer_active = timer->active = false;
+    } else {
+        gl->QueryCounter(timer->stop[timer->idx], GL_TIMESTAMP);
+    }
 
+    timer->idx = (timer->idx + 1) % GL_QUERY_OBJECT_NUM;
     return timer->result;
 }
 
diff --git a/video/out/opengl/ra_gl.h b/video/out/opengl/ra_gl.h
index 08f0197433..334db69cfd 100644
--- a/video/out/opengl/ra_gl.h
+++ b/video/out/opengl/ra_gl.h
@@ -8,6 +8,7 @@
 struct ra_gl {
     GL *gl;
     bool debug_enable;
+    bool timer_active; // hack for GL_TIME_ELAPSED
 };
 
 // For ra_tex.priv

[haasn]

Google devs chose the same approach of Apple ones on this: google/angle@f04f667

Seems like their rationale is different though - complaining about lack of timestamps reported by D3D11. (Ironically contradicting what apple seems to have suggested)

[AirPort]

I can confirm that the patch above works as expected on my machine.

[haasn]

I'll leave it up to @wm4 and @rossy, in particular since this breaks on ANGLE as well.

[quilloss]

Recent changes seems to have made the "upload frame" timer extremely jittery. Is this due to the unreliability of GL_TIMESTAMP that Apple is complaining about.

[rossy]

@haasn Yeah, that works, but it's missing a definition for GL_EXT_disjoint_timer_query:

DEF_FN_NAME(GetQueryiv, "glGetQueryivEXT"),

[haasn]

@quilloss Not necessarily, it could just be the case that uploading the frames actually spikes now. It could also be the case that GL_TIMESTAMP is more accurate then GL_TIME_ELAPSED, rather than the other way around. Hard to tell.

[haasn]

I'll just revert it, since I can live with just using the global ra_gl.timer_active hack to avoid triggering the illegal behavior.

[Argon-]

(Thanks. I absolutely realize it's a pain in the ass to work around broken operating systems you don't even use yourself. I appreciate every effort made to support/provide features to a larger set of systems)

[haasn]

@Argon- Well, you're the one writing the stats script, which I love. Making it actually work on your system is the least I could do. :p