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

Implement can_play_type for ServoMedia #232

Merged
merged 1 commit into from Apr 16, 2019

Conversation

Projects
None yet
5 participants
@georgeroman
Copy link
Contributor

commented Apr 2, 2019

This is related to servo/servo#22299. I tried to follow WebKit's implementation of a similar function, as suggested by @ferjm in the issue's comments.

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch from c77bf28 to 081c1d0 Apr 2, 2019

@ceyusa

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

I would like rather to have a fixed list of mime-types that are possible to play in the web and check if the current gstreamer setup has support for them. Thus the backend won't traverse the whole gstreamer's registry per-call but rather a hash map to look up, just as in

https://github.com/WebKit/webkit/blob/0d960a0797d44e001af795b3d227be6b763fbbc0/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L2199

The return of the function perhaps should be an enum with Probably and Maybe

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thanks for the PR, @georgeroman

I agree with @ceyusa, we should keep a list of mime-types that are supported on the web (we can get it from the WebKit's implementation that @ceyusa shared), lazily intersect that list with what's available in the GStreamer registry and cache the result of that intersection so subsequent calls to this function do not require traversing the entire registry again.

@philn

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Please not the WebKit code linked here is obsolete, I implemented a new GstreamerRegistryScanner for media-capabilities support, it's used for canPlayType and the media-capabilities spec platform support.

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch 2 times, most recently from c37f073 to c1bead8 Apr 5, 2019

@georgeroman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

I added some of the requested changes. I would really appreciate some feedback to see whether I'm heading in the right direction with this!

@ferjm
Copy link
Member

left a comment

Great work! LGTM. I would like @ceyusa to take a second look though. Thanks!

Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch from c1bead8 to 64a47ef Apr 10, 2019

Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch from 64a47ef to beb364e Apr 10, 2019

@ferjm

ferjm approved these changes Apr 11, 2019

Copy link
Member

left a comment

Thanks!

Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch from beb364e to 82526d0 Apr 11, 2019

@ferjm

ferjm approved these changes Apr 12, 2019

Copy link
Member

left a comment

Nice! Just a couple of small changes more and this should be ready to merge. Thanks!

Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated

@georgeroman georgeroman force-pushed the georgeroman:implement_can_play_type branch from 82526d0 to caebc22 Apr 12, 2019

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Looks great! Thanks @georgeroman

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

📌 Commit caebc22 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

⌛️ Testing commit caebc22 with merge 5462b05...

bors-servo added a commit that referenced this pull request Apr 16, 2019

Auto merge of #232 - georgeroman:implement_can_play_type, r=ferjm
Implement can_play_type for ServoMedia

This is related to servo/servo#22299. I tried to follow WebKit's [implementation](https://github.com/WebKit/webkit/blob/ae5a8bdded113530fe844e01ad9c0e72ce85dbe2/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp#L304) of a similar function, as suggested by @ferjm in the issue's comments.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

☀️ Test successful - checks-travis
Approved by: ferjm
Pushing 5462b05 to master...

@bors-servo bors-servo merged commit caebc22 into servo:master Apr 16, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.