Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate RDK Telemetry #1445

Open
wants to merge 2 commits into
base: wpe-2.38
Choose a base branch
from

Conversation

marcin-mielczarczyk-red
Copy link

@marcin-mielczarczyk-red marcin-mielczarczyk-red commented Jan 16, 2025

Telemetry framework is designed to collect and transmit operational and diagnostic data from the set-top box for monitoring and debugging purposes.
It's main purpose is to report media playback state (drm type, media type, etc.) and graphics state.
By default dummy implementation is used which has no dependencies on any external libraries and doesn't send any reports.
Enabling USE_RDK_TELEMETRY option links RDK telemetry library and enables functionality of sending telemetry reports.
Telemetry module can be expanded to use other vendor specific telemetry functionalities.
cff168d

Build-Tests Layout-Tests
❌ 🛠 wpe-238-amd64-build ❌ 🧪 wpe-238-amd64-layout
❌ 🛠 wpe-238-arm32-build ❌ 🧪 wpe-238-arm32-layout

@@ -381,6 +381,11 @@ GLContextEGL::GLContextEGL(PlatformDisplay& display, EGLContext context, EGLSurf
}
RELEASE_ASSERT(!m_eglCreateImageKHR == !m_eglDestroyImageKHR);
}

if(m_type == WindowSurface) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the rest of the modified files should have USE(RDK_TELEMETRY) ifdefs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philn I guess you would like me to wrap all my changes around USE(RDK_TELEMETRY) ifdefs?
In such case ThirdParty/telemetry directory should be included only when USE(RDK_TELEMETRY) is defined and all calls to telemetry library should be wrapped with those ifdefs, like below:

#if USE(RDK_TELEMETRY) m_telemetry.reportPlaybackState(Telemetry::IReport::AVPipelineState::PLAYBACK_ERROR, std::string(err->message)); #endif

In case some other vendor would like to add its own specific implementation for telemetry, generic code will be polluted with more ifdefs.
In current proposal, vendor specific implementation is hidden in telemetry library. It's also cleaner to extend/add new vendor telemetry implementation.
I wanted to avoid adding new ifdefs in MediaPlayerPrivateGStreamer.cpp and GLContextEGL.cpp files, but if it's more preferable way, I'll do it.
Please, let me know your opinion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvaris suggested to use the runtime quirks for this... so ifdefs could be avoided, but likely the shim lib would have to remain here downstream.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @philn and @calvaris.

@marcin-mielczarczyk-red Could you please apply quirk mechanism to all codes coming into graphics/gl graphics/gstreamer. Right now I have seen "include"s are not controlled. And deep inlined with main code.

@philn If you have ideas how to integrate this ThirdParty code into base code, could you please share?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philn @modeveci @calvaris I pushed draft proposal for RDK Telemetry quirk in gstreamer.
It doesn't compile. It's just to check if I properly understand how this quirk should be implemented.

I don't see any quirk manager in 'graphics/egl'. Should I create a new one?

The last question is about location of RDK Telemetry implementation. I think 'ThirdParty' folder is a good place, because it would be common for telemetry integration in other directories, like 'WebKit' or 'WTF' (if there will be such need in the future).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants