Media elements improvements #8203

kajarenc · 2024-02-26T16:19:45Z

Describe your changes

Add end_time and loop parameters to st.audio and st.video
Specs:
End time https://www.notion.so/snowflake-corp/Spec-cf910f5811a84a6a9976d230c85a044e
Loop: https://www.notion.so/snowflake-corp/Spec-18bbbb288f4c4a0ba129d95eeb9c0260

GitHub Issue Link (if applicable)

Testing Plan

Explanation of why no additional tests are needed
Unit Tests (JS and/or Python) Done ✅
E2E Tests Done ✅
Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

frontend/lib/src/components/elements/Video/Video.tsx

snehankekre · 2024-03-04T10:07:09Z

@kajarenc The end time and looping behavior work as expected, according to the specs 😄 LGTM

LukasMasuch

LGTM 👍 What happens if the configured end time or start time is larger than the video time?

LukasMasuch · 2024-03-11T18:39:23Z

e2e_playwright/st_audio_test.py

+    audio_element.evaluate("el => el.play()")
+    app.wait_for_timeout(5000)
+    expect(audio_element).to_have_js_property("paused", True)
+    assert int(audio_element.evaluate("el => el.currentTime")) == 13


nit: To prevent flakiness we probably need to use wait_until method from conftest. See this as an example:

streamlit/e2e_playwright/st_graphviz_chart_test.py

Line 74 in b1dd9f8

wait_until(app, check_dimensions)

Also see https://github.com/streamlit/streamlit/wiki/Running-e2e-tests-and-updating-snapshots#three-rules-of-playwright :)

LukasMasuch · 2024-03-11T18:40:59Z

e2e_playwright/st_audio_test.py

+    # approximately 17 (4 seconds until end_time and 2 seconds starting from start time)
+    app.wait_for_timeout(6000)
+    expect(audio_element).to_have_js_property("paused", False)
+    assert 16 < audio_element.evaluate("el => el.currentTime") < 18


Same as above :)

LukasMasuch · 2024-03-11T18:45:33Z

e2e_playwright/st_video_test.py

+@pytest.mark.parametrize(
+    "nth_element",
+    [
+        pytest.param(5, marks=pytest.mark.skip_browser("webkit")),


interesting :) didn't know this trick 👍

LukasMasuch · 2024-03-11T19:00:51Z

e2e_playwright/st_video_test.py

+    # Wait until video will reach end_time
+    app.wait_for_timeout(3000)
+    expect(video_element).to_have_js_property("paused", True)
+    assert int(video_element.evaluate("el => el.currentTime")) == 33


Replace with wait_until (see comments above)

LukasMasuch · 2024-03-11T19:01:01Z

e2e_playwright/st_video_test.py

+    # 4 seconds until end_time and 2 seconds starting from start time
+    app.wait_for_timeout(6000)
+    expect(video_element).to_have_js_property("paused", False)
+    assert 36 < video_element.evaluate("el => el.currentTime") < 38


Replace with wait_until (see comments above)

LukasMasuch · 2024-03-11T19:50:15Z

frontend/lib/src/components/elements/Video/Video.tsx

+    if (loop) {
+      youtubeUrl.searchParams.append("loop", "1")
+      // When using the loop parameter, YouTube requires the playlist parameter to be set to the same video ID
+      const videoId = youtubeUrl.pathname.split("/").pop()


Does that work for all the different types of youtube URLs?

It should work with different youtube URLs, because we standardize URL on the backend

streamlit/lib/streamlit/elements/media.py

Line 268 in e1960fb

return f"https://www.youtube.com/embed/{code}"

LukasMasuch · 2024-03-11T19:51:39Z

lib/streamlit/elements/media.py

@@ -79,6 +81,10 @@ def audio(
        sample_rate: int or None
            The sample rate of the audio data in samples per second. Only required if
            ``data`` is a numpy array.
+        end_time: int
+            The time at which this element should stop playing.


nit: Maybe add that its the number of seconds

nit: in some other time-related parameters (e.g. ttl), we also support timedelta and str (parsed by Pandas) in addition to the number of seconds. Maybe also something to consider here:

This could work here as well, but maybe not the best fit

cc @snehankekre

The smallest reasonable measure to configure a video/audio start/end time is probably seconds or? So no need to use float here?

The smallest reasonable measure to configure a video/audio start/end time is probably seconds or? So no need to use float here?

@LukasMasuch The currentTime property of the HTMLMediaElement interface from which HTMLVideoElement inherits is specified in seconds according to the docs: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/currentTime

And it can be set or returned as "a double-precision floating-point value indicating the current playback time in seconds", so we can support float here. Should we do it in this PR or another one scoped specifically to this change?

we also support timedelta and str (parsed by Pandas) in addition to the number of seconds. Maybe also something to consider here

@LukasMasuch really good point! This is a suggestion Thiago also had when we presented these specs in office hours.

Supporting timedelta objects from datetime and a string specifying the time in a format supported by Pandas's Timedelta constructor, in addition to seconds, would be nice for consistency across the API and will reduce the learning curve for new users.

+1 to supporting them.

That would let devs do things like:

import streamlit as st from datetime import timedelta start_time_td = timedelta(seconds=15) end_time_td = timedelta(minutes=2, seconds=30) st.video(data="video.mp4", start_time=start_time_td, end_time=end_time_td) # Assuming in our internal implementation we call .total_seconds() on the timedelta objects

import streamlit as st start_time_str = "15s" end_time_str = "2m 30s" # These values are parsed using Pandas's Timedelta constructor st.video(data="video.mp4", start_time=start_time_str, end_time=end_time_str)

kajarenc · 2024-03-12T14:56:14Z

LGTM 👍 What happens if the configured end time or start time is larger than the video time?

We raise an exception if end_time < start_time, but for end time or start time is larger than video length handled by the browser (and behavior is very expected).

If start_time > video.length then the player on the last frame, but when you click play it starts from the beginning of the video.

If end_time > video.length the player stops on the last frame of the video (if loop == True, we continue from start_time).

Add end_time and loop parameters to st.audio and st.video

kajarenc added 2 commits February 26, 2024 20:18

Add loop parameter to st.audio

e7ec9da

fix failing test

8396ef5

kajarenc changed the title ~~[WIP] Add loop parameter to st.audio~~ [WIP] Media elements improvements Feb 26, 2024

kajarenc added 3 commits February 26, 2024 21:55

make mypy happy

cdef7b9

add end_time and loop parameters to st.video

2065a4d

Frontend changes

d5f5e67

github-advanced-security bot found potential problems Feb 29, 2024

View reviewed changes

frontend/lib/src/components/elements/Video/Video.tsx Fixed Show fixed Hide fixed

kajarenc added 3 commits February 29, 2024 20:46

better video ID handling on frontend

22e1d2b

make linter happy

7dcd853

fix end time behaviour

b8e1eaa

snehankekre added feature:st.audio feature:st.video change:feature impact:users labels Mar 6, 2024

kajarenc added 6 commits March 7, 2024 18:47

Merge branch 'develop' into media-improvements

a54605d

align st.audio behaviour with st.video

dd723c7

Merge branch 'develop' into media-improvements

a24eb2c

Add python unit tests

944c79a

Add e2e audio tests

2b62e18

Add e2e video tests

ff72890

kajarenc added the security-assessment-completed label Mar 11, 2024

better tests

a9d2e33

kajarenc changed the title ~~[WIP] Media elements improvements~~ Media elements improvements Mar 11, 2024

kajarenc marked this pull request as ready for review March 11, 2024 17:48

kajarenc requested review from vdonato, LukasMasuch and sfc-gh-mnowotka as code owners March 11, 2024 17:49

LukasMasuch approved these changes Mar 11, 2024

View reviewed changes

replace asserts with wait_until

dff537e

kajarenc merged commit 8162158 into develop Mar 13, 2024
35 of 36 checks passed

This was linked to issues Mar 14, 2024

Create parameter 'end_time' for st.video #6277

Closed

Loop option for video #3951

Closed

zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024

Media elements improvements (streamlit#8203)

c5a8f88

Add end_time and loop parameters to st.audio and st.video

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media elements improvements #8203

Media elements improvements #8203

kajarenc commented Feb 26, 2024 •

edited

snehankekre commented Mar 4, 2024

LukasMasuch left a comment •

edited

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

kajarenc Mar 12, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

LukasMasuch Mar 11, 2024

snehankekre Mar 12, 2024 •

edited

snehankekre Mar 12, 2024

snehankekre Mar 12, 2024

kajarenc commented Mar 12, 2024

Media elements improvements #8203

Media elements improvements #8203

Conversation

kajarenc commented Feb 26, 2024 • edited

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

snehankekre commented Mar 4, 2024

LukasMasuch left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehankekre Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kajarenc commented Mar 12, 2024

kajarenc commented Feb 26, 2024 •

edited

LukasMasuch left a comment •

edited

snehankekre Mar 12, 2024 •

edited