VPAAMP-959:[MP4DEMUX]Use codec info from the manifest rather than laz…

[varshnie]

…y caps setting in mp4demux

Reason for change:Use codec info from the manifest rather than lazy caps setting in mp4demux Risks: p1

[Copilot]

Pull request overview

This PR updates the GStreamer appsrc source configuration so that, when DRM protection data is already queued, the appsrc caps are rewritten to encrypted (application/x-cenc) caps up-front (instead of relying on later/lazy caps behavior), aligning better with using manifest-derived codec/encryption information.

Changes:

• Detect queued DRM protection events during InitializeSourceForPlayer() and decide whether encrypted caps should be applied.
• When applicable, mutate caps by setting original-media-type, optionally setting GST_PROTECTION_SYSTEM_ID_CAPS_FIELD, and renaming the caps to application/x-cenc.
• Add logging to help trace when encrypted caps are applied during source configuration.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp:843

• After deleting the global g_mockGstUtils, the pointer is left dangling. Because FakeGstUtils checks only for non-null before dispatching, any later call to GetCaps/CreateGstBufferWithData would become a use-after-free and can make tests flaky.

	delete g_mockGstUtils;

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

🟡 Changes recommended

The newly added unit tests reference a non-existent Configs::enableEncryptedCaps member and don’t set stream->codecInfo.mIsEncrypted, which will cause build/test failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp:849

• Configs has no enableEncryptedCaps member, so this will not compile. To exercise the non-encrypted path in InitializeSourceForPlayer, set stream->codecInfo.mIsEncrypted to false (default) instead.

	stream->format = GST_FORMAT_VIDEO_ES_H264;
	mPlayerContext->protectionEvent[mediaType] = reinterpret_cast<GstEvent*>(0x1234);
	mPlayerConfigParams->videoBufBytes = 500;
	mPlayerConfigParams->enableEncryptedCaps = false;

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The CLI (test/pi-cli/commandProcessing.cpp) calls ConfigurePipeline with the wrong argument list after the API change, which should cause a build failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

InterfacePlayerRDK.h:355

• The ConfigurePipeline API was extended with PipelineCodecInfo, but the doxygen block above the declaration wasn’t updated (missing @param codecInfo, and subFormat is described as a boolean). This makes the public API contract misleading for callers.

			void ConfigurePipeline(PipelineCodecInfo&& codecInfo, int format, int audioFormat, int subFormat, bool bESChangeStatus, bool setReadyAfterPipelineCreation, bool isSubEnable, int32_t trackId, gint rate, const char *pipelineName, int PipelinePriority, bool FirstFrameFlag, std::string url, bool enableLiveLatency = false);

InterfacePlayerRDK.cpp:314

• In the ConfigurePipeline doxygen comment, subFormat is still documented as a boolean (“Whether subtitle format is enabled”), but the parameter is an integer subtitle format value.

 * @brief Configures the GStreamer pipeline.
 * @param codecInfo Pipeline codec information.
 * @param format Video format.
 * @param audioFormat Audio format.
 * @param subFormat Whether subtitle format is enabled.
 * @param bESChangeStatus Whether ES change status is enabled.

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There is a confirmed CLI call-site compilation issue and a behavior regression risk in encrypted caps handling when the DRM system ID is unset.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

test/pi-cli/commandProcessing.cpp:215

• The updated configurepipeline CLI still calls ConfigurePipeline with the old argument order/arity (it passes bESChangeStatus where the API now expects the int video format). This will not compile against InterfacePlayerRDK::ConfigurePipeline(PipelineCodecInfo&&, int, int, int, ...).

        player.ConfigurePipeline(
            PipelineCodecInfo{
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(format)),
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(audioFormat)),
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(subFormat))},
            bESChangeStatus, setReadyAfterPipelineCreation, isSubEnable,
            trackId, rate, pipelineName, PipelinePriority, subBool, url
        );

InterfacePlayerRDK.h:355

• The ConfigurePipeline Doxygen block above this declaration wasn’t updated for the new PipelineCodecInfo parameter (it still starts with @param format). This makes the header documentation inconsistent with the actual API signature.

			void ConfigurePipeline(PipelineCodecInfo&& codecInfo, int format, int audioFormat, int subFormat, bool bESChangeStatus, bool setReadyAfterPipelineCreation, bool isSubEnable, int32_t trackId, gint rate, const char *pipelineName, int PipelinePriority, bool FirstFrameFlag, std::string url, bool enableLiveLatency = false);

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The pi-cli ConfigurePipeline call site is incompatible with the updated function signature (compile break), and a small state-consistency fix is advisable where stream->format and stream->codecInfo.mCodecFormat can diverge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

test/pi-cli/commandProcessing.cpp:212

• The CLI still calls ConfigurePipeline with the old parameter order (it omits the required format/audioFormat/subFormat arguments after the new PipelineCodecInfo), which will not compile against the updated InterfacePlayerRDK::ConfigurePipeline signature.

        player.ConfigurePipeline(
            PipelineCodecInfo{
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(format)),
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(audioFormat)),
                MediaCodecInfo(static_cast<GstStreamOutputFormat>(subFormat))},

InterfacePlayerRDK.h:355

• The ConfigurePipeline doxygen comment is now out of sync with the function signature: it documents format as the first parameter, but the function now takes PipelineCodecInfo&& codecInfo first.

			void ConfigurePipeline(PipelineCodecInfo&& codecInfo, int format, int audioFormat, int subFormat, bool bESChangeStatus, bool setReadyAfterPipelineCreation, bool isSubEnable, int32_t trackId, gint rate, const char *pipelineName, int PipelinePriority, bool FirstFrameFlag, std::string url, bool enableLiveLatency = false);

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A compile-blocking codec member reference and invalid-format track-counting logic remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Three unresolved findings remain in InterfacePlayerRDK.cpp, including missing manifest codec parameters and stale stream codec state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

InterfacePlayerRDK.cpp:456

• On a fresh player, every stream starts with GST_FORMAT_INVALID; when the incoming format is also invalid, isInitialSetup is true and this expression still marks the stream for reconfiguration. That increments NumberOfTracks for all three absent tracks (and makes the new invalid-format test fail), so the initial-setup branch must only apply when the new format is valid.

		bool shouldReconfigure = isInitialSetup || isFormatChanged;

InterfacePlayerRDK.cpp:5632

• SetStreamCaps still only updates stream->format; it does not persist the rvalue codecInfo into the new stream->codecInfo member. After this new InitializeSourceForPlayer logic is used to recreate a source, an encrypted stream configured through SetStreamCaps can be initialized with clear caps (or the reverse) because mIsEncrypted is stale. Keep the stream's codec-info state synchronized when this API updates the caps.

		if (codecInfo.mIsEncrypted)
		{
			TransformToEncryptedCaps(caps, mDrmSystem);

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Five unresolved moderate findings remain in codec state, caps configuration, reconfiguration, and track-count handling.

Review details

Suppressed comments (5)

InterfacePlayerPriv.h:128

• This new per-stream field is not cleared when a stream is torn down: TearDownStream resets format but leaves codecInfo intact. After an encrypted subtitle is torn down, a subsequent no-subtitle tune can create the closed-caption control stream while mIsEncrypted is still true, causing InitializeSourceForPlayer to transform application/x-subtitle-cc into application/x-cenc. Reset codecInfo alongside format during teardown.

	MediaCodecInfo codecInfo; /**< Codec and encryption information for this stream */

InterfacePlayerRDK.cpp:461

• Encryption-only changes now enter this branch, but this increments NumberOfTracks even when stream->format is unchanged. NumberOfTracks is used to detect single-track audio playback (for example at lines 3895 and 4493), so toggling encryption on an existing track can make the count 2 and suppress the audio-only first-frame path. Increment the count only when a new/different format is added, preserving the previous behavior for encryption-only reconfiguration.

			interfacePlayerPriv->gstPrivateContext->NumberOfTracks++;

InterfacePlayerRDK.cpp:1979

• The new overload stores the full MediaCodecInfo, but source setup still creates caps from only stream->format; this block uses only mIsEncrypted. Consequently mCodecData and mInfo (video dimensions and audio rate/channels) from the manifest are ignored, so this path cannot replace the codec-specific caps previously supplied by Mp4Demux::setCaps/SetStreamCaps and appsrc may lack required codec fields. Apply the stored codec information when building caps before setting them on the source.

		if (stream->codecInfo.mIsEncrypted)
		{
			MW_LOG_DEBUG("Applying encrypted caps during source configuration for type[%d] format[%d]", mediaType, stream->format);
			TransformToEncryptedCaps(caps, mDrmSystem);

InterfacePlayerRDK.cpp:5632

• SetStreamCaps applies the encryption transform but never persists the supplied codecInfo in stream->codecInfo. Since InitializeSourceForPlayer now consults that stored flag, a stream configured through this API can have encrypted caps initially and then be recreated with stale mIsEncrypted == false, causing clear caps to be advertised for encrypted samples. Store/move the codec info into the stream on every caps setup, including the unencrypted case.

			TransformToEncryptedCaps(caps, mDrmSystem);

InterfacePlayerRDK.cpp:456

• The reconfiguration predicate only considers the format and mIsEncrypted. If a manifest update keeps the same format but changes codec data, dimensions, or audio properties, this path skips the move at line 502, leaving the previous stream->codecInfo and caps in place. Include the codec metadata in change detection or update the stored state and caps whenever it changes.

		bool isEncryptionChanged = (stream->codecInfo.mIsEncrypted != codecInfoByTrack[i]->mIsEncrypted);
		bool isFormatChanged = (stream->format != newFormat[i] || isEncryptionChanged);
		// Reconfigure pipeline if this is the first setup, or the encryption status or format has changed
		bool shouldReconfigure = isValidNewFormat && (isInitialSetup || isFormatChanged);

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate correctness, lifecycle, and buffer-ownership findings must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

InterfacePlayerPriv.h:128

• codecInfo is new per-stream state, but teardown still only resets stream->format (InterfacePlayerRDK.cpp:1450). After an encrypted subtitle stream is stopped, a later Rialto closed-caption setup can inherit mIsEncrypted=true; the new InitializeSourceForPlayer check then rewrites application/x-subtitle-cc to application/x-cenc. Reset this member with the rest of the stream state, or exclude the closed-caption path from encrypted-cap transformation.

InterfacePlayerRDK.cpp:461

• When only mIsEncrypted changes, shouldReconfigure is true even though the same logical track remains active, and this unconditional increment changes NumberOfTracks from one to two. That value gates audio-only and first-frame behavior at InterfacePlayerRDK.cpp:3902 and 4500, so an encryption transition can suppress those callbacks; increment it only when a previously invalid track is added.

			interfacePlayerPriv->gstPrivateContext->NumberOfTracks++;

InterfacePlayerRDK.cpp:456

• The new overload accepts the complete MediaCodecInfo, but this decision only compares the format and encryption bit. If a manifest period or representation keeps H.264/AAC while changing codec data, dimensions, or audio parameters, the setup is skipped and stream->codecInfo and its caps remain from the previous track, so the decoder can receive stale configuration. Include the relevant codec fields in the change test or force stream setup when they change.

		bool isEncryptionChanged = (stream->codecInfo.mIsEncrypted != codecInfoByTrack[i]->mIsEncrypted);
		bool isFormatChanged = (stream->format != newFormat[i] || isEncryptionChanged);
		// Reconfigure pipeline if this is the first setup, or the encryption status or format has changed
		bool shouldReconfigure = isValidNewFormat && (isInitialSetup || isFormatChanged);

InterfacePlayerRDK.cpp:1979

• stream->codecInfo is not cleared by TearDownStream, and invalid new formats are deliberately not moved into it. After an encrypted subtitle stream is torn down, the closed-caption control stream can therefore inherit mIsEncrypted == true and this new branch converts its application/x-subtitle-cc caps to application/x-cenc. Clear the codec metadata when disabling/tearing down a stream, or exclude the control-stream caps from this transform.

		if (stream->codecInfo.mIsEncrypted)
		{
			MW_LOG_DEBUG("Applying encrypted caps during source configuration for type[%d] format[%d]", mediaType, stream->format);
			TransformToEncryptedCaps(caps, mDrmSystem);

InterfacePlayerRDK.cpp:5639

• After this helper is used, SetStreamCaps still never persists its consumed codecInfo into stream->codecInfo. A caller using this existing API with an encrypted track can set encrypted caps here, but any later InitializeSourceForPlayer() sees the default clear-stream flag and recreates clear caps; store the codec info before relying on this state.

			TransformToEncryptedCaps(caps, mDrmSystem);

InterfacePlayerRDK.cpp:1979

• The new manifest MediaCodecInfo is only consulted here through mIsEncrypted; the source setup still obtains generic caps from GetCaps(stream->format), so mCodecData and mInfo (dimensions, channels, and sample rate) are never applied in this path. A manifest H.264/AAC track therefore still needs a separate SetStreamCaps call to receive codec-specific caps, which defeats the stated replacement of lazy MP4-demux caps setup. Apply the full codec info when creating source caps or route this setup through the existing codec-capability logic.

		if (stream->codecInfo.mIsEncrypted)
		{
			MW_LOG_DEBUG("Applying encrypted caps during source configuration for type[%d] format[%d]", mediaType, stream->format);
			TransformToEncryptedCaps(caps, mDrmSystem);

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Five unresolved moderate findings (one vote each) remain in InterfacePlayerRDK.cpp, affecting track selection, codec metadata, encryption state, track counts, and caps initialization.

Review details

Suppressed comments (5)

InterfacePlayerRDK.cpp:456

• When a track switch keeps the same codec format and encryption state, this predicate is false, so the setup block never assigns the new trackId at line 502. stream->trackId is later read when the audio decoder reaches READY (the AC4 presentation selection path), so a setReadyAfterPipelineCreation/track-ID change can continue using the previous AC4 track. Include the track-ID change in the reconfiguration condition or update it before decoder setup.

		bool isEncryptionChanged = (stream->codecInfo.mIsEncrypted != codecInfoByTrack[i]->mIsEncrypted);
		bool isFormatChanged = (stream->format != newFormat[i] || isEncryptionChanged);
		// Reconfigure pipeline if this is the first setup, or the encryption status or format has changed
		bool shouldReconfigure = isValidNewFormat && (isInitialSetup || isFormatChanged);

InterfacePlayerRDK.cpp:456

• This predicate only detects format and encryption changes, but MediaCodecInfo also carries codec data and dimensions/audio parameters. When those manifest values change while the format stays the same, the condition is false and the assignment at line 502 is skipped, leaving stream->codecInfo (and therefore the caps configuration) stale. Compare the codec metadata that affects caps or refresh the stored info/caps even when the format is unchanged.

		bool isEncryptionChanged = (stream->codecInfo.mIsEncrypted != codecInfoByTrack[i]->mIsEncrypted);
		bool isFormatChanged = (stream->format != newFormat[i] || isEncryptionChanged);
		// Reconfigure pipeline if this is the first setup, or the encryption status or format has changed
		bool shouldReconfigure = isValidNewFormat && (isInitialSetup || isFormatChanged);

InterfacePlayerRDK.cpp:461

• An encryption-only reconfiguration increments NumberOfTracks even though no track was added. NumberOfTracks is used as the current track count by NotifyFirstFrame and audio-only handling, so an audio-only stream changing from unencrypted to encrypted can go from 1 to 2 and fail to emit its first-frame notification. Increment this counter only for initial setup or a format change, not when only isEncryptionChanged is true.

		if(shouldReconfigure)
		{
			MW_LOG_MIL("Closing stream %d old format = %d, new format = %d",i, stream->format, newFormat[i]);
			configureStream[i] = true;
			interfacePlayerPriv->gstPrivateContext->NumberOfTracks++;

InterfacePlayerRDK.cpp:336

• The retained integer-format overload now fabricates codec metadata with mIsEncrypted == false. If a stream was configured through the new overload as encrypted and a caller subsequently uses this public compatibility overload, the comparison at line 453 treats that as an encryption transition and reconfigures the stream with unencrypted metadata, dropping the DRM caps signaling. Preserve the existing encryption state for this legacy path or keep encryption-change detection disabled when codec metadata was not supplied.

	StreamCodecInfo streamInfo;
	streamInfo.video.mCodecFormat = static_cast<GstStreamOutputFormat>(format);
	streamInfo.audio.mCodecFormat = static_cast<GstStreamOutputFormat>(audioFormat);
	streamInfo.subtitle.mCodecFormat = static_cast<GstStreamOutputFormat>(subFormat);
	ConfigurePipeline(std::move(streamInfo), bESChangeStatus, setReadyAfterPipelineCreation,

InterfacePlayerRDK.cpp:1979

• This new initialization path only uses mIsEncrypted; it never applies the manifest's mCodecData or mInfo (dimensions, sample rate, and channel count) to the caps. Those fields are applied only by SetStreamCaps, so if mp4demux switches from that lazy call to the new ConfigurePipeline(StreamCodecInfo&&) path as described by this PR, H.264/AAC caps are created without codec configuration and the decoder can fail or lose stream parameters. Reuse the codec-field enrichment when initializing these caps, or retain an equivalent SetStreamCaps step.

		if (stream->codecInfo.mIsEncrypted)
		{
			MW_LOG_DEBUG("Applying encrypted caps during source configuration for type[%d] format[%d]", mediaType, stream->format);
			TransformToEncryptedCaps(caps, mDrmSystem);

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite