Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd Player as a crate #88
Conversation
|
Nice! Looks good overall. I left some comments and questions. @Manishearth could you take a look as well, please? Thanks! |
| let height = s.get("height").unwrap(); | ||
| let buffer = sample.get_buffer().unwrap(); | ||
|
|
||
| let map = buffer.map_readable().unwrap(); |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
We need to remove all these unwraps. Make this function return a Result<Frame, ()>, please.
Also, nit: rename the function to from_sample
| let dur = media_info.get_duration(); | ||
| let duration = if dur != gst::ClockTime::none() { | ||
| let nanos = dur.nanoseconds().unwrap() % 1_000_000_000; | ||
|
|
This comment has been minimized.
This comment has been minimized.
| format: format, | ||
| audio_tracks: audio_tracks, | ||
| video_tracks: video_tracks, | ||
| } |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
nit: we can use field init shorthand here:
Metadata {
duration,
width,
height,
format,
audio_tracks,
video_tracks,
}| let mut video_tracks = Vec::new(); | ||
| if let Some(f) = media_info.get_container_format() { | ||
| format = f; | ||
| } |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
let format = match media_info.get_container_format() {
Some(f) => f,
None => "".to_owned(),
};| struct GStreamerMetadata {} | ||
|
|
||
| impl GStreamerMetadata { | ||
| fn new(media_info: &gst_player::PlayerMediaInfo) -> Metadata { |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
Same as before, there are some unwraps that we need to remove here. Return Result<Metadata, ()>. And possibly rename the function as from_media_info.
|
|
||
| impl GStreamerPlayer { | ||
| pub fn new() -> GStreamerPlayer { | ||
| let player = gst_player::Player::new(None, None); |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
nit:
let player = gst_player::Player::new(/* video renderer */ None, /* signal dispatcher */ None);| fn set_input_size(&self, size: u64) { | ||
| if let Ok(mut inner) = self.inner.lock() { | ||
| inner.set_input_size(size); | ||
| } |
This comment has been minimized.
This comment has been minimized.
|
|
||
| pub fn start(&mut self) { | ||
| self.player.pause(); | ||
| } |
This comment has been minimized.
This comment has been minimized.
|
|
||
| fn set_input_size(&self, size: u64); | ||
| fn push_data(&self, data: Vec<u8>) -> bool; | ||
| fn end_of_stream(&self) -> bool; |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
Result<(), ()> as the return value of setup, push_data and end_of_stream is probably more idiomatic and let us extend it with specific errors in the future if needed.
| } else { | ||
| unreachable!() | ||
| } | ||
| } |
This comment has been minimized.
This comment has been minimized.
ferjm
Jul 18, 2018
Member
Not required for this PR, but it would be really nice if we could do something like gecko-media's test-player :-)
| struct GStreamerFrame {} | ||
|
|
||
| impl GStreamerFrame { | ||
| fn new(sample: &gst::Sample) -> Frame { |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 18, 2018
Member
This can just be a function, i think, no need for the GStreamerFrame type
| } | ||
| } | ||
|
|
||
| struct GStreamerMetadata {} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 18, 2018
Member
Alternatively, this can be a From impl, impl From<&PlayerMediaInfo> for Metadata
| appsrc: Option<gst_app::AppSrc>, | ||
| appsink: gst_app::AppSink, | ||
| input_size: u64, | ||
| subscribers: Vec<ipc::IpcSender<PlayerEvent>>, |
This comment has been minimized.
This comment has been minimized.
ceyusa
force-pushed the
ceyusa:player
branch
3 times, most recently
from
5189295
to
0d491d4
ceyusa commentedJul 17, 2018
This pull request is rebased above #87