-
-
Notifications
You must be signed in to change notification settings - Fork 828
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
FLV support #10756
FLV support #10756
Conversation
this is very cool! A defunct website I recreate mostly uses FLVs for the intros and others, so I can't wait to see it working! Nice! |
Progresses http://labcenter.dnalc.org/labs/dnaextraction/dnaextraction_d.html (uses VP6A and MP3 codecs with external FLV files), which used to say:
It now says:
There is no functional difference between what used to happen on that site and what happens now though, unfortunately. Progresses #10173 (uses H.264 codec with external MP4 files), number 2, which used to say:
It now says:
There is no functional difference between what used to happen on that site and what happens now though, unfortunately. Progresses #10175, number 2, which used to say:
It now says:
There is no functional difference between what used to happen on that site and what happens now though, unfortunately. Progresses #10175, number 3 in the same way described for #10173, number 2. |
Fixes #10705. |
Should progress the flv issue for #10149? |
2832e76
to
ee5c75e
Compare
I think the reason there's no functional difference currently is because the FLV PR isn't yet merged into the |
But wasn't the cargo lock changed to point to kmeisthax's latest commit? |
Daniel is correct, |
You can turn it into a real draft by clicking the "Still in progress? Convert to draft" text below Reviewers - Suggestions, which is on the top-right of the page in Github. |
b6850c6
to
fc16132
Compare
I love how that link is buried where you can't find it |
FWIW you can also switch the "Open PR" button to open as a draft right away. |
1a4e411
to
eb4de1b
Compare
.into()) | ||
.into(); | ||
|
||
ns.set_avm_object(activation.context.gc_context, this.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I do not know if I like the cyclic reference between the Object
and NetStream
, but it might be fine, and alternatives, like a callback, might be overall worse.
movie: Arc<SwfMovie>, | ||
|
||
/// The self bounds for this movie. | ||
size: (i32, i32), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Might this be better as (u32, 32)
? Might be better as it currently is.
FlvTagData::Audio(FlvAudioData { .. }) => { | ||
tracing::warn!("Stub: Stream audio processing"); | ||
} | ||
FlvTagData::Video(FlvVideoData { codec_id, data, .. }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is somewhat long. Would it be helpful (or worse) to maybe put parts of this function into their own functions that is called from here? Like, a function that might be called handle_flv_tag_data_video()
for this match?
But, one problem with that is how it would interfere with the loop
, in case there are return
or break
or continue
statements and the like. The usage of a loop
, together with a lot of code, feels a bit difficult to handle to me, though I could be wrong, both on this point and on possible alternatives on organizing the code being sufficiently better to change to one of them.
It is obvious that a lot of time and effort went into this PR. I know too little of Rust, Ruffle and video playing to really give good feedback on this, but I have tried to leave a few comments. The PR is also rather long, making it more difficult to review. Parts that I think would be good to review is the overall type design and interface between the different parts of the codebase, and maybe also a light review of parts of the implementation, though this is of course only an initial PR and many parts have not been implemented yet or are works in progress. @kmeisthax What do you think? Are there any specific aspects and parts of the PR that you would like to have reviewed? I have not really reviewed this, so it is still in need of reviewing. |
https://www.nothingtodo.co.uk/view/3136/who-needs-a-glove.html logs "Video has no decoded frame to render" with this PR. |
Since DisplayObjects use instance allocators now, this needs to define an instance allocator on |
…_frame`. You can also specify a `tick_rate` in order to get ticks faster or slower than the stage frame rate. We also enable video decode and ticks on the netstream tests, since our netstream impl requires ticks.
…e allocator function
…re the actual video data.
…atible-ish datastream so that we can still play the video
I posted a different Screen V1 test FLV on Discord, which will hopefully be less sensitive to platform differences. |
… display object are connected correctly
This PR adds FLV support. Notably, only video: the existing streaming audio APIs want to work with either SWF bitstreams or single contiguous blocks of MP3 data. That needs refactoring big enough to justify a separate PR.
FLV also, bizarrely, has functionality for calling ActionScript methods from the video stream. Yes, the
onMetaData
event is actually triggered by one of these script calls; and no I did not implement it yet. I still do need to parse this block in order to not violate the video backend's existing API prerequisites, though.This can't be merged until ruffle-rs/h263-rs#50 is merged and this PR is updated to target the merged version.I rolled that PR into this one so it can be merged in one go.