Skip to content

dd: feat: multi-res support #746

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

dd: feat: multi-res support #746

wants to merge 6 commits into from

Conversation

gioelecerati
Copy link
Member

@gioelecerati gioelecerati commented Jun 2, 2025

PR Type

Enhancement


Description

Record both input and output clips
Add input clip download/share button
Adjust aspect ratios for mobile portrait
Use mobile pipeline & resolution params


Changes walkthrough 📝

Relevant files
Enhancement
9 files
ClipButton.tsx
Add input clip props and mobile record logic                         
+10/-1   
ClipModal.tsx
Support input clip in modal state                                               
+17/-1   
ClipShareContent.tsx
Add input clip download/share UI                                                 
+74/-12 
types.ts
Extend ClipData with input clip fields                                     
+2/-0     
broadcast.tsx
Use portrait aspect ratio on mobile                                           
+1/-1     
Dreamshaper.tsx
Switch Dreamshaper player to 9:16 on mobile                           
+1/-1     
ManagedBroadcast.tsx
Apply portrait layout in collapsed mobile view                     
+1/-1     
useDreamshaper.tsx
Add mobile pipeline and resolution handling                           
+72/-7   
useVideoClip.ts
Record input stream alongside output clip                               
+54/-6   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    vercel bot commented Jun 2, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    pipelines-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2025 7:36am

    Copy link

    github-actions bot commented Jun 2, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 0a1b9a1)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Parameter Mismatch

    The call to recordClip("output-only", null) passes null as the second argument,
    but recordClip immediately calls inputVideo.captureStream(). This will throw
    if inputVideo is null.

    if (isMobile) {
      recordClip("output-only", null);
    } else {
      showRecordingOptions();
    }
    Unconditional Input Recording

    In recordClip, the input media recorder is always started regardless of the selected
    recording mode. This may result in unwanted input recordings when only output clips
    are desired.

    // Always record the input video separately
    const inputVideoStream = inputVideo.captureStream();
    const inputMediaRecorder = new MediaRecorder(inputVideoStream, {
      mimeType,
    });
    
    const chunks: Blob[] = [];
    const inputChunks: Blob[] = [];
    
    mediaRecorder.ondataavailable = e => {

    Copy link

    github-actions bot commented Jun 2, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 0a1b9a1
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Guard inputVideo null reference

    Prevent a runtime error when inputVideo is null by only initializing
    inputMediaRecorder if an element was passed in. Wrap the capture and recorder setup
    in a conditional check.

    apps/app/hooks/useVideoClip.ts [221-225]

    -// Always record the input video separately
    -const inputVideoStream = inputVideo.captureStream();
    -const inputMediaRecorder = new MediaRecorder(inputVideoStream, {
    -  mimeType,
    -});
    +// Always record the input video separately if provided
    +let inputMediaRecorder: MediaRecorder | undefined;
    +if (inputVideo) {
    +  const inputVideoStream = inputVideo.captureStream();
    +  inputMediaRecorder = new MediaRecorder(inputVideoStream, {
    +    mimeType,
    +  });
    +}
    Suggestion importance[1-10]: 8

    __

    Why: The unguarded use of inputVideo.captureStream() can cause a runtime crash if inputVideo is null, so adding a conditional check prevents errors while recording input.

    Medium
    Use dynamic recorder count

    Make the completion threshold dynamic based on whether inputMediaRecorder exists, so
    the modal always appears. Compute expectedRecorders and compare against it instead
    of hardcoding 2.

    apps/app/hooks/useVideoClip.ts [241-249]

    +const expectedRecorders = inputMediaRecorder ? 2 : 1;
     let recordingComplete = 0;
     const checkComplete = () => {
       recordingComplete++;
    -  if (recordingComplete === 2) {
    +  if (recordingComplete === expectedRecorders) {
         setShowClipModal(true);
         setIsRecording(false);
         setProgress(0);
       }
     };
    Suggestion importance[1-10]: 7

    __

    Why: Hardcoding recordingComplete === 2 breaks the modal when only one recorder exists, so dynamically computing the expected recorder count ensures the clip modal always appears correctly.

    Medium
    General
    Check canShare before share

    Verify that the device can share files using navigator.canShare before calling
    navigator.share to avoid unhandled promise rejections on unsupported platforms.

    apps/app/components/daydream/Clipping/ClipShareContent.tsx [185-189]

    -await navigator.share({
    -  title: "Daydream Input Clip",
    -  text: "Original input clip from Daydream",
    -  files: [file],
    -});
    +if (navigator.canShare && navigator.canShare({ files: [file] })) {
    +  await navigator.share({
    +    title: "Daydream Input Clip",
    +    text: "Original input clip from Daydream",
    +    files: [file],
    +  });
    +} else {
    +  // Fallback or notify user
    +}
    Suggestion importance[1-10]: 6

    __

    Why: Verifying navigator.canShare before calling navigator.share avoids unhandled promise rejections on unsupported platforms and improves robustness.

    Low
    Remove null argument

    Drop the unnecessary null argument when calling recordClip so it only takes the
    recording mode. The hook already handles undefined options.

    apps/app/components/daydream/Clipping/ClipButton.tsx [91-95]

     if (isMobile) {
    -  recordClip("output-only", null);
    +  recordClip("output-only");
     } else {
       showRecordingOptions();
     }
    Suggestion importance[1-10]: 4

    __

    Why: Dropping the unnecessary null argument simplifies the recordClip call without affecting functionality since the hook handles undefined inputs.

    Low

    Previous suggestions

    Suggestions up to commit 9f35c7b
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Defer pipeline selection

    Compute pipelineId inside the initialization useEffect after confirming
    initialIsMobile is not null. This avoids selecting the desktop pipeline on first
    render and prevents the effect from early returning due to initialIsMobile being
    null.

    apps/app/hooks/useDreamshaper.tsx [636-641]

    -const pipelineId =
    -  searchParams.get("pipeline_id") ||
    -  (initialIsMobile
    -    ? process.env.NEXT_PUBLIC_SHOWCASE_MOBILE_PIPELINE_ID ||
    -      DEFAULT_MOBILE_PIPELINE_ID
    -    : process.env.NEXT_PUBLIC_SHOWCASE_PIPELINE_ID || DEFAULT_PIPELINE_ID);
    +useEffect(() => {
    +  if (capacityLoading || initialIsMobile === null) return;
    +  const pipelineId =
    +    searchParams.get("pipeline_id") ||
    +    (initialIsMobile
    +      ? process.env.NEXT_PUBLIC_SHOWCASE_MOBILE_PIPELINE_ID ||
    +        DEFAULT_MOBILE_PIPELINE_ID
    +      : process.env.NEXT_PUBLIC_SHOWCASE_PIPELINE_ID || DEFAULT_PIPELINE_ID);
    +  // existing fetchData logic...
    +}, [pathname, ready, user, searchParams, capacityLoading, hasCapacity, initialIsMobile]);
    Suggestion importance[1-10]: 7

    __

    Why: Moving the pipelineId calculation into the useEffect ensures initialIsMobile is settled before choosing between mobile or desktop pipelines, preventing an incorrect default on first render.

    Medium
    General
    Clamp mobile dimensions

    Enforce upper limits on mobile dimensions (e.g., maxWidth=1080, maxHeight=1920)
    after parsing to prevent extremely large or invalid sizes from being applied.

    apps/app/hooks/useDreamshaper.tsx [503-513]

    -const widthValue = parseFloat(commands["width"]);
    +const widthValue = parseFloat(commands["width"] ?? "");
     if (!isNaN(widthValue) && widthValue > 0) {
    -  updatedInputValues.prompt["16"].inputs.width = widthValue;
    +  updatedInputValues.prompt["16"].inputs.width = Math.min(widthValue, 1080);
     }
    -const heightValue = parseFloat(commands["height"]);
    +const heightValue = parseFloat(commands["height"] ?? "");
     if (!isNaN(heightValue) && heightValue > 0) {
    -  updatedInputValues.prompt["16"].inputs.height = heightValue;
    +  updatedInputValues.prompt["16"].inputs.height = Math.min(heightValue, 1920);
     }
    Suggestion importance[1-10]: 5

    __

    Why: Adding upper bounds for width and height helps prevent excessively large values, improving robustness without altering core logic.

    Low
    Use explicit mobile check

    Use an explicit comparison (initialIsMobile === true) to ensure the mobile block
    only runs when initialIsMobile is actually true, avoiding accidental execution when
    it's null or false.

    apps/app/hooks/useDreamshaper.tsx [490-515]

    -if (initialIsMobile) {
    +if (initialIsMobile === true) {
       // mobile-specific inputs
     }
    Suggestion importance[1-10]: 3

    __

    Why: The block already only runs when initialIsMobile is true (since null and false are falsy), so the explicit comparison adds little value beyond readability.

    Low

    Copy link

    github-actions bot commented Jun 5, 2025

    Persistent review updated to latest commit 0a1b9a1

    Comment on lines +531 to +556
    if (initialIsMobile) {
    if (!updatedInputValues.prompt["16"]) {
    updatedInputValues.prompt["16"] = { inputs: {} };
    }
    if (!updatedInputValues.prompt["16"].inputs) {
    updatedInputValues.prompt["16"].inputs = {};
    }

    updatedInputValues.prompt["16"].inputs.width = 384;
    updatedInputValues.prompt["16"].inputs.height = 704;
    updatedInputValues.prompt["16"].inputs.batch_size = 1;

    if (commands["width"]) {
    const widthValue = parseFloat(commands["width"]);
    if (!isNaN(widthValue) && widthValue > 0) {
    updatedInputValues.prompt["16"].inputs.width = widthValue;
    }
    }

    if (commands["height"]) {
    const heightValue = parseFloat(commands["height"]);
    if (!isNaN(heightValue) && heightValue > 0) {
    updatedInputValues.prompt["16"].inputs.height = heightValue;
    }
    }
    }
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Instead of the exact prompt ID #, we should query for the node with a class_type of EmptyLatentImage

    {
        "_meta": {
          "title": "Empty Latent Image"
        },
        "inputs": {
          "width": 704,
          "height": 384,
          "batch_size": 1
        },
        "class_type": "EmptyLatentImage"
    }
    

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

    Successfully merging this pull request may close these issues.

    2 participants