RDKEMW-6776 Encrypted Pipeline Changes with SVPPAY on AAMP - #196
Open
suryaiyappan2k wants to merge 1 commit into
Open
RDKEMW-6776 Encrypted Pipeline Changes with SVPPAY on AAMP#196suryaiyappan2k wants to merge 1 commit into
suryaiyappan2k wants to merge 1 commit into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is intended to adjust encrypted playback pipeline behavior (per description: insert svppayload immediately after aampdecryptor), and it also refactors DRM decryptor src-pad caps advertisement to be built dynamically based on a runtime-discovered platform secure-memory feature.
Changes:
- Adds a new
element_setupcallback hookup for video streams inInterfacePlayerRDK(currently only logs element names). - Builds Widevine/PlayReady decryptor src pad-template caps dynamically at
class_inittime using newgst_cdmidecryptor_*helper APIs. - Adds runtime discovery of platform secure-memory caps feature in
gstcdmidecryptorand updates caps-transform logging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| InterfacePlayerRDK.cpp | Adds a new element_setup callback for video streams (currently logging-only). |
| gst-plugins/drm/gst/gstwidevinedecryptor.cpp | Replaces static src pad-template caps with dynamically generated caps string. |
| gst-plugins/drm/gst/gstplayreadydecryptor.cpp | Same dynamic src pad-template caps approach as Widevine. |
| gst-plugins/drm/gst/gstcdmidecryptor.h | Exposes new public APIs for platform memory-feature discovery and caps-string building. |
| gst-plugins/drm/gst/gstcdmidecryptor.cpp | Implements platform memory-feature discovery, dynamic mime-type/caps generation, and adds additional caps-transform logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2071
to
+2075
| static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data) | ||
| { | ||
| const gchar *name = gst_element_get_name(element); | ||
| MW_LOG_ERR("muthumani: Received element:%s", name); | ||
| } |
Comment on lines
+2071
to
+2075
| static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data) | ||
| { | ||
| const gchar *name = gst_element_get_name(element); | ||
| MW_LOG_ERR("muthumani: Received element:%s", name); | ||
| } |
Comment on lines
+2521
to
+2524
| if (eGST_MEDIATYPE_VIDEO == streamId) | ||
| { | ||
| g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK); | ||
| } |
| @@ -127,6 +127,183 @@ | |||
|
|
|||
| static const gchar *srcMimeTypes[] = { "video/x-h264", "video/x-h264(memory:SecMem)", "audio/mpeg", "video/x-h265", "video/x-h265(memory:SecMem)", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3","audio/x-opus", nullptr }; | |||
Comment on lines
+131
to
+135
| static const gchar *baseMimeTypes[] = { | ||
| "video/x-h264", "video/x-h265", | ||
| "audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus", | ||
| nullptr | ||
| }; |
Comment on lines
+635
to
+652
| if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps)) | ||
| { | ||
| gchar *caps_before = gst_caps_to_string(transformedCaps); | ||
| g_print("muthumani: OCDMGstTransformCaps - caps BEFORE transform: %s\n", caps_before); | ||
| g_free(caps_before); | ||
|
|
||
| if (OCDMGstTransformCaps) | ||
| { | ||
| OpenCDMError ret = OCDMGstTransformCaps(&transformedCaps); | ||
| gchar *caps_after = gst_caps_to_string(transformedCaps); | ||
| g_print("muthumani: OCDMGstTransformCaps - called, ret=%d, caps AFTER transform: %s\n", ret, caps_after); | ||
| g_free(caps_after); | ||
| } | ||
| else | ||
| { | ||
| g_print("muthumani: OCDMGstTransformCaps is NULL - skipping caps transform\n"); | ||
| } | ||
| } |
Comment on lines
+86
to
+93
| const gchar *platformMemFeature = gst_cdmidecryptor_get_platform_memory_feature(); | ||
| gchar *srcCapsStr = gst_cdmidecryptor_build_src_caps_string(platformMemFeature); | ||
| GstCaps *srcCaps = gst_caps_from_string(srcCapsStr); | ||
| g_free(srcCapsStr); | ||
|
|
||
| GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps); | ||
| gst_caps_unref(srcCaps); | ||
| gst_element_class_add_pad_template(elementClass, srcTemplate); |
Comment on lines
+89
to
+97
| const gchar *platformMemFeature = gst_cdmidecryptor_get_platform_memory_feature(); | ||
| gchar *srcCapsStr = gst_cdmidecryptor_build_src_caps_string(platformMemFeature); | ||
| GstCaps *srcCaps = gst_caps_from_string(srcCapsStr); | ||
| g_free(srcCapsStr); | ||
|
|
||
| GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps); | ||
| gst_caps_unref(srcCaps); | ||
| gst_element_class_add_pad_template(elementClass, srcTemplate); | ||
|
|
Comment on lines
+90
to
+91
| * The returned string is allocated with g_strdup_printf and must be freed with g_free(). | ||
| * Typically called from a subclass class_init to construct a dynamic pad template. |
Comment on lines
+130
to
+135
| /* Base video/audio media types supported by all platforms */ | ||
| static const gchar *baseMimeTypes[] = { | ||
| "video/x-h264", "video/x-h265", | ||
| "audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus", | ||
| nullptr | ||
| }; |
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 10, 2026 09:14
c4f02be to
4f76608
Compare
Contributor
Contributor
Contributor
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 14, 2026 04:28
4f76608 to
ecc97aa
Compare
| @@ -127,6 +127,182 @@ | |||
|
|
|||
| static const gchar *srcMimeTypes[] = { "video/x-h264", "video/x-h264(memory:SecMem)", "audio/mpeg", "video/x-h265", "video/x-h265(memory:SecMem)", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3","audio/x-opus", nullptr }; | |||
Comment on lines
+636
to
+648
| gchar *caps_before = gst_caps_to_string(transformedCaps); | ||
| g_free(caps_before); | ||
|
|
||
| if (OCDMGstTransformCaps) | ||
| { | ||
| OpenCDMError ret = OCDMGstTransformCaps(&transformedCaps); | ||
| gchar *caps_after = gst_caps_to_string(transformedCaps); | ||
| g_free(caps_after); | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING("OCDMGstTransformCaps is NULL - skipping caps transform"); | ||
| } |
Comment on lines
+91
to
+93
| GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps); | ||
| gst_caps_unref(srcCaps); | ||
| gst_element_class_add_pad_template(elementClass, srcTemplate); |
Comment on lines
+94
to
+96
| GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps); | ||
| gst_caps_unref(srcCaps); | ||
| gst_element_class_add_pad_template(elementClass, srcTemplate); |
| * @brief Build a dynamic GstCaps string for the src pad template of a decryptor, | ||
| * appending the platform-specific secure memory feature (if any) to video types. | ||
| * | ||
| * The returned string is allocated with g_strdup_printf and must be freed with g_free(). |
Comment on lines
+2071
to
+2075
| static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data) | ||
| { | ||
| const gchar *name = gst_element_get_name(element); | ||
| MW_LOG_INFO( "Received element: %s", name); | ||
| } |
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 14, 2026 04:36
ecc97aa to
c39b353
Compare
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 19, 2026 13:59
c39b353 to
e834710
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
InterfacePlayerRDK.cpp:2075
- The new element_setup_cb_svppay logs at INFO for every element_setup invocation, which can spam logs and add overhead. Consider lowering the log level and/or filtering to the specific element(s) you care about (e.g., the decryptor) and mark unused parameters explicitly.
static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data)
{
const gchar *name = gst_element_get_name(element);
MW_LOG_INFO( "Received element: %s", name);
}
InterfacePlayerRDK.cpp:2524
- PR description says svppayload should be inserted immediately after aampdecryptor in the encrypted pipeline, but the current change only connects a callback that logs element names and does not perform any insertion/relinking. Either implement the insertion here (e.g., when the decryptor element is created) or update the PR description to match the actual behavior.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
gst-plugins/drm/gst/gstcdmidecryptor.cpp:133
- The dynamically-built baseMimeTypes list dropped audio/x-ac4, but both Widevine and PlayReady sink templates still advertise audio/x-ac4. This can break caps negotiation for AC4 content (regression vs the previous static src caps).
static const gchar *baseMimeTypes[] = {
"video/x-h264", "video/x-h265",
"audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus",
nullptr
};
gst-plugins/drm/gst/gstcdmidecryptor.cpp:640
- This block creates caps strings (and a ret variable) but immediately frees/ignores them, which is wasted work and may trigger -Wunused-variable warnings in builds that treat warnings as errors. Either remove it entirely or log the values via GST_* macros.
gchar *caps_before = gst_caps_to_string(transformedCaps);
g_free(caps_before);
if (OCDMGstTransformCaps)
{
gst-plugins/drm/gst/gstwidevinedecryptor.cpp:92
- gst_caps_from_string() can return NULL; the current code unrefs srcCaps unconditionally, which can crash and/or fail to register the src pad template. Add a fallback (e.g., retry without the platform memory feature or use gst_caps_new_any()) before creating the pad template.
GstCaps *srcCaps = gst_caps_from_string(srcCapsStr);
g_free(srcCapsStr);
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst-plugins/drm/gst/gstplayreadydecryptor.cpp:95
- gst_caps_from_string() can return NULL; the current code unrefs srcCaps unconditionally, which can crash and/or fail to register the src pad template. Add a fallback (e.g., retry without the platform memory feature or use gst_caps_new_any()) before creating the pad template.
GstCaps *srcCaps = gst_caps_from_string(srcCapsStr);
g_free(srcCapsStr);
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst-plugins/drm/gst/gstcdmidecryptor.h:91
- The docstring says the returned string is allocated with g_strdup_printf, but the implementation uses GString/g_string_free(). Update the comment to avoid misleading callers (the important part is that g_free() must be used).
* The returned string is allocated with g_strdup_printf and must be freed with g_free().
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 30, 2026 02:15
e834710 to
8c93e1c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
InterfacePlayerRDK.cpp:2524
- This PR’s description says it "inserts the svppayload element into encrypted pipeline immediately after the aampdecryptor element", but the only change here is connecting an element_setup callback that logs element names. There is no code that actually inserts/links an svppayload element; either implement the insertion logic in this callback (or elsewhere) or update the PR description to match the change.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
gst-plugins/drm/gst/gstcdmidecryptor.cpp:133
- The dynamic src pad caps (used by Widevine/PlayReady) are built from baseMimeTypes, but audio/x-ac4 is missing even though both decryptors’ sink pad templates accept audio/x-ac4. This drops AC4 from the advertised decrypted output caps compared to the previous static src template, which can break caps negotiation for AC4 streams.
/* Base video/audio media types supported by all platforms */
static const gchar *baseMimeTypes[] = {
"video/x-h264", "video/x-h265",
"audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus",
nullptr
};
gst-plugins/drm/gst/gstcdmidecryptor.cpp:302
- This g_print() emits the generated caps string unconditionally. Prefer GST_DEBUG/GST_TRACE (or remove) to avoid noisy stdout output in production.
g_print("surya - gst_cdmidecryptor_build_src_caps_string: capsStr: %s\n", capsStr->str);
gst-plugins/drm/gst/gstcdmidecryptor.cpp:646
- This block adds unconditional g_print() output and allocates caps_before/caps_after strings (and a ret variable) that are immediately freed/unused. This adds overhead during caps negotiation without any functional effect. Remove the unused conversions/variables and keep only the conditional OCDMGstTransformCaps call (with the NULL guard).
g_print("surya - gst_cdmidecryptor_transform_caps: transformedCaps: %" GST_PTR_FORMAT, transformedCaps);
if (socInterface && socInterface->IsTransformCapsRequired())
{
if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps))
{
gchar *caps_before = gst_caps_to_string(transformedCaps);
g_free(caps_before);
if (OCDMGstTransformCaps)
{
OpenCDMError ret = OCDMGstTransformCaps(&transformedCaps);
gchar *caps_after = gst_caps_to_string(transformedCaps);
g_free(caps_after);
InterfacePlayerRDK.cpp:2075
- The new element_setup_cb_svppay() is currently only emitting a per-element log line (with a personal tag) and will run for every element in every video pipeline, which can create excessive log noise. Consider either removing this callback or restricting it to the specific element(s) you care about, and use an existing debug log level/message without personal identifiers.
static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data)
{
const gchar *name = gst_element_get_name(element);
MW_LOG_INFO( "surya - Received element: %s", name);
}
gst-plugins/drm/gst/gstwidevinedecryptor.cpp:95
- Using g_print() in a GStreamer element class_init writes directly to stdout/stderr and bypasses the project’s logging controls. Replace this with GST_* logging (or remove it) and drop the personal tag.
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst_element_class_add_pad_template(elementClass, srcTemplate);
g_print("surya - widevinedecryptor: setting dynamic src pad template caps");
gst-plugins/drm/gst/gstplayreadydecryptor.cpp:98
- This log is emitted at GST_WARNING level during class init and includes a personal tag. It should be downgraded to debug/info (or removed) to avoid polluting logs at warning severity.
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst_element_class_add_pad_template(elementClass, srcTemplate);
GST_WARNING("surya - playreadydecryptor: setting dynamic src pad template caps");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:225
- The new g_print() statement bypasses GStreamer logging and will print unconditionally during feature discovery. Use GST_* logging (or remove) so output is controllable via debug levels.
This issue also appears on line 301 of the same file.
g_print("surya - gst_cdmidecryptor_discover_platform_memory_feature");
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 30, 2026 04:23
8c93e1c to
fe95957
Compare
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 30, 2026 07:16
fe95957 to
6424155
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (10)
InterfacePlayerRDK.cpp:2538
- The PR description says SVPPAY should be inserted into the encrypted pipeline immediately after the
aampdecryptorelement, but the added code only connects anelement_setuphandler that (currently) just logs element names. There is no logic here (or elsewhere in this PR) that actually inserts/links ansvppayloadelement or restricts the behavior to encrypted playback, so the change as-is doesn't meet the stated goal.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_print("surya - patch applied: SVPPAY callback started\n");
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
gst-plugins/drm/gst/gstcdmidecryptor.cpp:132
baseMimeTypesno longer includesaudio/x-ac4, but existing decryptors’ sink templates still advertise AC4 encrypted input (e.g. PlayReady/Widevine) and other decryptors’ src templates include AC4. Dropping AC4 from the shared src caps/mime list can break caps negotiation for AC4 content.
static const gchar *baseMimeTypes[] = {
"video/x-h264", "video/x-h265",
"audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus",
nullptr
gst-plugins/drm/gst/gstcdmidecryptor.cpp:289
- This function prints to stdout during decryptor class initialization, which makes startup logs noisy and bypasses the project’s logging controls. Please remove the
g_print()(or convert to GST_* logging if truly needed).
g_print("surya - Patch applied for gst_cdmidecryptor_build_src_caps_string\n");
gst-plugins/drm/gst/gstcdmidecryptor.h:92
- The doc comment says the returned string is allocated with
g_strdup_printf, but the implementation usesGString+g_string_free(..., FALSE). The important contract is that the caller mustg_free()it; the allocator detail here is incorrect/misleading.
* The returned string is allocated with g_strdup_printf and must be freed with g_free().
* Typically called from a subclass class_init to construct a dynamic pad template.
InterfacePlayerRDK.cpp:2088
element_setup_cb_svppay()callsgst_element_get_name()but never frees the returned string (elsewhere in this filegst_element_get_name()is freed viag_free, e.g.SafeName()/element_setup_cb). This leaks on every element setup, and the currentg_print("Patch applied...")/ personal-tag logging is noisy for production.
static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data)
{
const gchar *name = gst_element_get_name(element);
g_print("Patch applied: surya - element_setup_cb_svppay\n");
MW_LOG_INFO( "surya - Received element: %s", name);
}
InterfacePlayerRDK.cpp:2538
- This adds an unconditional
g_print()in the video SetupStream path.g_print()writes to stdout and is typically not appropriate for production playback (it can spam logs and impact performance); the project already uses MW_LOG_* for controlled logging.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_print("surya - patch applied: SVPPAY callback started\n");
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
gst-plugins/drm/gst/gstcdmidecryptor.cpp:225
gst_cdmidecryptor_discover_platform_memory_feature()unrefsprobeCapsunconditionally. SinceOCDMGstTransformCaps(&probeCaps)is allowed to mutate the pointer,probeCapscan become NULL, which would makegst_caps_unref(probeCaps)crash.
gst_caps_unref(probeCaps);
if (!platformSecureMemFeature)
{
platformSecureMemFeature = g_strdup(""); /* no special memory feature on this platform */
GST_INFO("gst_cdmidecryptor: no platform-specific memory feature detected");
}
gst-plugins/drm/gst/gstwidevinedecryptor.cpp:96
gst_widevinedecryptor_class_init()usesg_print()for debug output (and the second message repeats "caps1"). This writes directly to stdout and can leak into production logs; prefer GST_* logging or remove entirely.
const gchar *platformMemFeature = gst_cdmidecryptor_get_platform_memory_feature();
gchar *srcCapsStr = gst_cdmidecryptor_build_src_caps_string(platformMemFeature);
GstCaps *srcCaps = gst_caps_from_string(srcCapsStr);
g_free(srcCapsStr);
g_print("surya - widevinedecryptor: setting dynamic src pad template caps1\n");
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst_element_class_add_pad_template(elementClass, srcTemplate);
g_print("surya - widevinedecryptor: setting dynamic src pad template caps1\n");
gst-plugins/drm/gst/gstplayreadydecryptor.cpp:99
gst_playreadydecryptor_class_init()usesg_print()for debug output. This writes directly to stdout and can leak into production logs; prefer GST_* logging or remove entirely.
const gchar *platformMemFeature = gst_cdmidecryptor_get_platform_memory_feature();
gchar *srcCapsStr = gst_cdmidecryptor_build_src_caps_string(platformMemFeature);
GstCaps *srcCaps = gst_caps_from_string(srcCapsStr);
g_free(srcCapsStr);
g_print("surya - playreadydecryptor: setting dynamic src pad template caps1\n");
GstPadTemplate *srcTemplate = gst_pad_template_new("src", GST_PAD_SRC, GST_PAD_ALWAYS, srcCaps);
gst_caps_unref(srcCaps);
gst_element_class_add_pad_template(elementClass, srcTemplate);
g_print("surya - playreadydecryptor: setting dynamic src pad template caps2\n");
gst_element_class_add_static_pad_template(elementClass, &gst_playreadydecryptor_sink_template);
gst-plugins/drm/gst/gstcdmidecryptor.cpp:270
- This public helper prints to stdout every time it's called. Since decryptor class_init calls this, it will add noisy, non-configurable logging in production; please remove the
g_print()(or convert to GST_* logging if truly needed).
This issue also appears on line 289 of the same file.
g_print("surya - Patch applied for gst_cdmidecryptor_get_platform_memory_feature\n");
| "audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus", | ||
| nullptr | ||
| }; | ||
| g_print("surya - Patch applied for baseMimeTypes\n"); |
Comment on lines
+638
to
+642
| g_print("surya - gst_cdmidecryptor_transform_caps: transformedCaps: %" GST_PTR_FORMAT, transformedCaps); | ||
| if (socInterface && socInterface->IsTransformCapsRequired()) | ||
| { | ||
| if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps) && OCDMGstTransformCaps) | ||
| OCDMGstTransformCaps(&transformedCaps); | ||
| } | ||
| if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps)) | ||
| { |
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 30, 2026 07:28
6424155 to
0efebb8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
gst-plugins/drm/gst/gstcdmidecryptor.cpp:134
- The statement
g_print("surya - Patch applied for baseMimeTypes\n");is at global scope (outside any function), which will not compile in C++ (it’s not a valid declaration). Remove it or move it into a function guarded by an appropriate debug macro.
g_print("surya - Patch applied for baseMimeTypes\n");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:132
baseMimeTypesdroppedaudio/x-ac4, but both Widevine and PlayReady sink pad templates still advertiseoriginal-media-type=(string)audio/x-ac4. Withoutaudio/x-ac4in the decryptor src caps list, negotiation/output for AC-4 streams may fail/regress.
static const gchar *baseMimeTypes[] = {
"video/x-h264", "video/x-h265",
"audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus",
nullptr
gst-plugins/drm/gst/gstcdmidecryptor.cpp:642
- This block uses multiple
g_print()calls plusgst_caps_to_string()allocations inside caps negotiation, which adds noise and unnecessary overhead. It also computesOpenCDMError retbut never checks/logs it, which can hide transform failures (and may trigger unused-variable warnings under-Werror). PreferGST_TRACE/DEBUGwithGST_PTR_FORMATand handle non-ERROR_NONEreturns.
g_print("surya - gst_cdmidecryptor_transform_caps: transformedCaps: %" GST_PTR_FORMAT, transformedCaps);
if (socInterface && socInterface->IsTransformCapsRequired())
{
if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps))
{
InterfacePlayerRDK.cpp:2538
- PR description says SVPPAY payload element should be inserted into the encrypted pipeline immediately after
aampdecryptor, but this change only adds a secondelement_setupcallback that logs element names (and the repo has nosvppayload/aampdecryptorreferences). As-is, nothing here performs an insertion or enforces ordering in the pipeline.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_print("surya - patch applied: SVPPAY callback started\n");
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
InterfacePlayerRDK.cpp:2088
gst_element_get_name()is treated elsewhere in this file as returning an allocated string that must be freed (seeSafeName()which callsg_free(elementName)). Here the result is stored asconst gchar*and never freed, which is inconsistent and risks leaking. Also the callback prints directly to stdout and leaves parameters unused (can fail builds with-Werror=unused-parameter).
static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data)
{
const gchar *name = gst_element_get_name(element);
g_print("Patch applied: surya - element_setup_cb_svppay\n");
MW_LOG_INFO( "surya - Received element: %s", name);
InterfacePlayerRDK.cpp:2536
- Avoid
g_print()in the player path; it writes directly to stdout and bypasses the project logging controls. This looks like temporary patch tracing and should be removed (or replaced with an existing MW_LOG_* level if truly needed).
g_print("surya - patch applied: SVPPAY callback started\n");
gst-plugins/drm/gst/gstwidevinedecryptor.cpp:101
- The new
g_print()calls in class_init write directly to stdout and will spam logs on every plugin load. Prefer GStreamer logging (GST_DEBUG/INFO/WARNING) so output is controllable by GST_DEBUG levels, or remove these messages entirely.
g_print("surya - widevinedecryptor: setting dynamic src pad template caps1\n");
gst-plugins/drm/gst/gstplayreadydecryptor.cpp:104
- The new
g_print()calls in class_init write directly to stdout and will spam logs on every plugin load. Prefer GStreamer logging (GST_DEBUG/INFO/WARNING) so output is controllable by GST_DEBUG levels, or remove these messages entirely.
g_print("surya - playreadydecryptor: setting dynamic src pad template caps1\n");
Reason for change: To insert the svppayload element into encrypted pipeline immediately after the aampdecryptor element. Test procedure: Test playback across all the apps. Risks: Low.
suryaiyappan2k
force-pushed
the
feature/RDKEMW-6776-v1
branch
from
July 30, 2026 08:12
0efebb8 to
8a9332d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (16)
gst-plugins/drm/gst/gstcdmidecryptor.cpp:133
- The dynamically built decryptor src caps no longer include
audio/x-ac4, which was present in the previous static src pad templates (widevine/playready). This can break negotiation for encrypted AC-4 streams.
/* Base video/audio media types supported by all platforms */
static const gchar *baseMimeTypes[] = {
"video/x-h264", "video/x-h265",
"audio/mpeg", "audio/x-eac3", "audio/x-gst-fourcc-ec_3", "audio/x-ac3", "audio/x-opus",
nullptr
};
gst-plugins/drm/gst/gstcdmidecryptor.cpp:641
gst_cdmidecryptor_transform_caps()is on a hot path; the addedg_print()and unconditionalgst_caps_to_string()allocations will add significant overhead and log spam. Also, theretvalue fromOCDMGstTransformCaps()is currently unused.
g_print("surya - gst_cdmidecryptor_transform_caps: transformedCaps: %" GST_PTR_FORMAT, transformedCaps);
if (socInterface && socInterface->IsTransformCapsRequired())
{
if (direction == GST_PAD_SINK && !gst_caps_is_empty(transformedCaps))
{
gst-plugins/drm/gst/gstcdmidecryptor.cpp:156
platformSecureMemFeatureis allocated and retained in a static without any cleanup; if the plugin can be unloaded, this becomes a leak.
static gchar *platformSecureMemFeature = nullptr;
gst-plugins/drm/gst/gstcdmidecryptor.cpp:235
- Avoid
g_print()for internal diagnostics; preferGST_DEBUG()/GST_TRACE()so it can be enabled selectively.
g_print("surya - patch applied for gst_cdmidecryptor_init_src_mime_types\n");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:269
- This public helper should not unconditionally print to stdout when called; use GStreamer debug logging if needed.
g_print("surya - Patch applied for gst_cdmidecryptor_get_platform_memory_feature\n");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:288
- Avoid unconditional
g_print()here; it will spam stdout during type/class initialization and makes logs noisy.
g_print("surya - Patch applied for gst_cdmidecryptor_build_src_caps_string\n");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:357
- Avoid
g_print()(stdout) for patch markers; useGST_DEBUG()instead so it can be controlled via debug configuration.
g_print("surya - Patch applied for gst_cdmidecryptor_class_init\n");
gst-plugins/drm/gst/gstcdmidecryptor.cpp:442
- Avoid
g_print()in init paths; preferGST_DEBUG_OBJECT()so logging is scoped to the element instance and can be enabled/disabled via GST_DEBUG.
g_print("surya - Patch applied for gst_cdmidecryptor_init\n");
InterfacePlayerRDK.cpp:2088
- Remove patch-marker
g_print()/personal-name logging fromelement_setup_cb_svppayand avoid leaving a callback that only emits stdout noise. If this callback is intended for debugging, use MW_LOG_DEBUG and keep it quiet by default.
static void element_setup_cb_svppay(GstElement *playbin, GstElement *element, gpointer user_data)
{
const gchar *name = gst_element_get_name(element);
g_print("Patch applied: surya - element_setup_cb_svppay\n");
MW_LOG_INFO( "surya - Received element: %s", name);
}
InterfacePlayerRDK.cpp:2538
- PR description says the change is to "insert the svppayload element into encrypted pipeline immediately after the aampdecryptor element", but the current change only connects an
element_setupcallback and logs element names. There is no svppayload element creation/insertion here, and no reference to aampdecryptor/svppayload in the codebase, so the described functional change does not appear to be implemented.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_print("surya - patch applied: SVPPAY callback started\n");
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
gst-plugins/drm/gst/gstcdmidecryptor.h:92
- The allocation detail in the doc comment is incorrect:
gst_cdmidecryptor_build_src_caps_string()currently builds the string viaGStringandg_string_free(..., FALSE), notg_strdup_printf(). The ownership/freeing guidance is correct, but the allocator reference should be updated to avoid misleading maintainers.
* The returned string is allocated with g_strdup_printf and must be freed with g_free().
* Typically called from a subclass class_init to construct a dynamic pad template.
gst-plugins/drm/gst/gstcdmidecryptor.cpp:150
srcMimeTypesArray/srcMimeTypesare allocated as process-lifetime statics and never released; repeated plugin load/unload (or test harnesses that dlclose) can leak memory.
This issue also appears on line 156 of the same file.
static GPtrArray *srcMimeTypesArray = nullptr;
static const gchar **srcMimeTypes = nullptr;
gst-plugins/drm/gst/gstcdmidecryptor.cpp:166
- Avoid
g_print()in library/plugin code (writes to stdout and bypasses GStreamer logging). Use GST_* macros so logging can be controlled via debug levels.
This issue also appears in the following locations of the same file:
- line 235
- line 269
- line 288
- line 357
- line 442
g_print("surya - gst_cdmidecryptor_discover_platform_memory_feature\n");
gst-plugins/drm/gst/gstwidevinedecryptor.cpp:101
- Avoid
g_print()in plugin code; use GST_* logging. Also, ifgst_caps_from_string()fails, the element ends up without a src pad template, which can break pad creation/negotiation; a fallback template is safer.
g_print("surya - widevinedecryptor: setting dynamic src pad template caps1\n");
if (srcCaps)
{
gst-plugins/drm/gst/gstplayreadydecryptor.cpp:105
- Avoid
g_print()in plugin code; use GST_* logging. Also ensure the src pad template is always registered even ifgst_caps_from_string()fails (fallback to ANY caps or a known-safe set).
g_print("surya - playreadydecryptor: setting dynamic src pad template caps1\n");
if (srcCaps)
{
InterfacePlayerRDK.cpp:2538
- Avoid unconditional
g_print()during stream setup; it writes to stdout and can pollute logs in production.
if (eGST_MEDIATYPE_VIDEO == streamId)
{
g_print("surya - patch applied: SVPPAY callback started\n");
g_signal_connect(stream->sinkbin, "element_setup", G_CALLBACK(element_setup_cb_svppay), pInterfacePlayerRDK);
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for change: To insert the svppayload element into encrypted pipeline immediately after the aampdecryptor element.
Test procedure: Test playback across all the apps.
Risks: Low.