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

fix(dgw): fix streamer failure for RDM #1184

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Jan 10, 2025

Changelog: ignore

Copy link

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

@irvingoujAtDevolution irvingoujAtDevolution changed the title streamer fix fix(dgw): fix streamer failure for RDM Jan 10, 2025
author irving ou <[email protected]> 1736528540 -0500
committer irving ou <[email protected]> 1736530356 -0500

fix(dgw): fix streamer failure for RDM
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Please apply this: https://github.com/Devolutions/IronRDP/blob/dd249909a894004d4f728d30b3a4aa77a0f8193b/STYLE.md#logging

I understand CutBlockHitMarker is a type token for ensuring we are only calling mark_cut_block_hit at most once per call to into_encoded_writer. This token is stored in an Option slot and taken out when webm_itr.last_tag_position() == cut_block_position.

I’m also not grokking fully this part as cut_block_position is itself the result of calling webm_itr.last_tag_position() prior to iterating? Suggesting that webm_itr.last_tag_position() == cut_block_position is always true. Am I missing something?

The problem was that mark_cut_block_hit() was called more than once, but why is it happening exactly? Breaking out of the loop and calling the function without relying on a dynamic state (via the Option) would be much more clean for instance.

crates/video-streamer/src/streamer/tag_writers.rs Outdated Show resolved Hide resolved
crates/video-streamer/src/streamer/tag_writers.rs Outdated Show resolved Hide resolved
@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Jan 11, 2025

When you asked why the "cut block" would be hit twice, it was a great question. Initially, I wasn’t entirely sure what was happening. While making this PR, I observed changes in the value of the tag emitted by the iterator. Specifically, during iteration, the tag iterator would emit a Cluster::Start master tag, and later cause the last_emitted_tag_offset to reset to zero. This behavior confused me at first, but I didn’t dive too deeply into it because the tag writer ignores anything that isn’t a frame. As long as the "cut block" isn’t repeatedly marked as hit, the functionality remains correct.

Answer to Your Question

The issue is related to a commit I made to the ebml_iterable repository:
Commit Link: 805e26bb35cd9b801c3ce977f69f2b1ceeaa9727.

Explanation

When the end of a file (EOF) is reached, ebml_iterable emits the enclosing end tags. However, in streaming scenarios, this behavior is undesirable. To address this, I asked the library author if we could have an option to disable this behavior, and they agreed.

However, this leads to a problem in a specific case: What happens if we aren’t reading from a Master::Start tag to begin with?

In this situation:

  • If the starting point is not a Master::Start tag (e.g., within a Cluster::Start or Segment::Start), and
  • We allow the emission of master end tags upon EOF,
    then the iterator will emit a Master::Start tag, and the last_emitted_tag_offset value will reset to zero.

Reproducing the Issue

Here’s a short Rust example to reproduce this scenario:

use std::fs::File;
use std::io::SeekFrom;

fn main() {
    let mut src = File::open(r"C:\ProgramData\Devolutions\Gateway\recordings\d2644cb0-63c3-412d-a040-cf4cb8b042e9\recording-0.webm").unwrap();
    src.seek(SeekFrom::Start(138)).unwrap();

    let mut tag_iterator = WebmIterator::new(&mut src, &[MatroskaSpec::BlockGroup(Master::Start)]);
    tag_iterator.emit_master_end_when_eof(true);

    println!("Start");
    while let Some(tag) = tag_iterator.next() {
        let name = webm_cutter::debug::mastroka_spec_name(&tag.unwrap());
        let offset = tag_iterator.last_emitted_tag_offset();
        println!("Tag: {:?}, offset = {}", name, offset);
    }
}

Output

Start
Tag: "Simple Block", offset = 0
Tag: "Simple Block", offset = 5356
Tag: "Simple Block", offset = 22625
Tag: "Simple Block", offset = 30576
Tag: "Simple Block", offset = 37829
Tag: "Simple Block", offset = 45326
Tag: "Simple Block", offset = 51293
Tag: "Simple Block", offset = 59010
Tag: "Simple Block", offset = 66221
Tag: "Cluster Start", offset = 0 <---- This causes a problem
Tag: "Segment Start", offset = 0

Solution

The fix is simple: disable the emission of master end tags when EOF is reached by adding this line:

tag_iterator.emit_master_end_when_eof(false);

This ensures that the WebmIterator behaves correctly when recreated in the rolling back process


Keep the Marker

Keeping the marker is still important because the "cut block" should only ever be hit once. This behavior ensures correctness in the streaming and cutting logic.

@irvingoujAtDevolution
Copy link
Contributor Author

I will address other concerns in the comments

@irvingoujAtDevolution
Copy link
Contributor Author

Oh, yes, and last_tag_position will change when we iterating over the file, so it is not always true.

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.

2 participants