Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

[[ WindowsPlayer ]] Update the player object on Windows to use DirectShow #4022

Merged
merged 43 commits into from
May 19, 2016

Conversation

livecodeian
Copy link
Contributor

@livecodeian livecodeian commented May 17, 2016

Modified MCPlatformPlayer to return the native view, using MCNativeLayer to manage geometry / visibility changes.
Implemented new MCPlatformPlayer based on DirectShow - switched Windows engine to use MCPlayer based on MCPlatformPlayer.


This change is Reviewable

livecodeian and others added 30 commits April 25, 2016 12:24
…Bitmap and bool return value to indicate success.

[[ WindowsPlayer ]] Modify MCPlayer::draw() to pass required image size when locking bitmap for offscreen rendering.
…m-internal.h

[[ WindowsPlayer ]] Rename mac-player.mm to platform-player.cpp and remove mac-specific includes.
…ise exclusions to allow Win32 builds to include required MCPlatform sources
…atures, and enable platform player feature on win32
[[ WIndowsPlayer ]]  Add video window proc to reconfigure video window on resize.
[[ WindowsPlayer ]] Fix video window class not registered
…+reattach native view.

[[ WindowsPlayer ]] Fix infinite loop when MCPlayer::playstop() returns false due to being uninitialised
…g view shown / hidden

[[ WindowsPlayer ]] Add property accessor stubs
@peter-b
Copy link
Contributor

peter-b commented May 18, 2016

Reviewed 1 of 22 files at r2, 2 of 3 files at r4.
Review status: 54 of 55 files reviewed at latest revision, 7 unresolved discussions.


docs/dictionary/property/callbacks.lcdoc, line 13 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

It should be possible to add in - there's already a timer event to track currenttime while playing. That can be expanded to check currenttime against the list of callbacks.

Okay, cool -- I'll add it to the backlog.

engine/src/mac-qt-player.mm, line 284 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

Each class defines m_view differently. The only reason the code is the same here is that the different types can all be cast to void.

OK. Is it worth considering a `NativeControl` template class?

engine/src/mac-qt-player.mm, line 966 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

With QuickTime support deprecated I don't think it's worth spending the time to clean up the header declarations.

OK, fair enough.

engine/src/w32-ds-player.cpp, line 1 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

I had pushed out a branch that was behind by 1 commit - have fixed this now.

The usual practice in GPL headers when there is shared copyright is to write something like:
Copyright (C) 2016 LiveCode Ltd.
Copyright (C) 2016 Docas AG.

I think that this is fine how it is however, thank you!


engine/src/w32-ds-player.cpp, line 89 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

The units depend on the time format used by the filter graph, as reported by IMediaSeeking->GetTimeFormat() so can't be known ahead of time. That's the point of the GetTimeScale method, which returns the number of units per second. I'm not sure what more I can do here.

In the graphics library there's an `MCGFloat` type which abstracts whether the graphics are being calculated using single- or double-precision floating point arithmetic -- and it's used to construct all the other graphics geometry types.

It's probably not necessary to do anything about this; I was just curious whether adding typedef uint32_t MCMediaPosition would make the code clearer, but from what you say perhaps it wouldn't make any difference.


engine/src/w32-ds-player.cpp, line 426 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

OAHWND is just a typedef for LONG_PTR. This appears to be standard Windows boilerplate for passing a HWND to an OLE object.

Thanks, that makes sense!

engine/src/w32-ds-player.cpp, line 618 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

I'll add the checks in. Assuming reference time units (100 nanoseconds) the maximum duration that can be stored in a uint32_t is ~119.3 hours, or just shy of 5 days :) Let's hope none of our users have video files that long.

Knowing our users, I wouldn't put it past them! I do think that the overflow checks are necessary. Do you think it makes sense to make the conversion saturating (i.e. if the duration is longer than `UINT32_MAX`, convert it to `UINT32_MAX`? Or is it better to just fail and return `false`?

engine/src/w32stack.cpp, line 339 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

I've pushed a new commit that does as you suggest - this will affect all controls with native layers, not just players

LGTM, thank you.

Comments from Reviewable

@peter-b
Copy link
Contributor

peter-b commented May 18, 2016

Reviewed 6 of 7 files at r5.
Review status: 54 of 55 files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@livecodeian
Copy link
Contributor Author

Review status: 54 of 55 files reviewed at latest revision, 4 unresolved discussions.


docs/dictionary/property/callbacks.lcdoc, line 13 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Okay, cool -- I'll add it to the backlog.

Just a note that there may be an IReferenceClock within the filter graph that can be used instead

engine/src/mac-qt-player.mm, line 284 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

OK. Is it worth considering a NativeControl<ViewType> template class?

At the moment that seems unnecessary. It may be something to consider during further development of MCPlatformPlayer.

engine/src/platform.h, line 1026 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

BTW is there a suitable "dimension" struct type that could be used here?

Yup, I've swapped this out with MCGIntegerSize

engine/src/w32-ds-player.cpp, line 618 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Knowing our users, I wouldn't put it past them! I do think that the overflow checks are necessary. Do you think it makes sense to make the conversion saturating (i.e. if the duration is longer than UINT32_MAX, convert it to UINT32_MAX? Or is it better to just fail and return false?

I'd rather it fail than return an incorrect value.

engine/src/w32-ds-player.cpp, line 1081 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Would it be appropriate to set r_value to an appropriate "default" value and/or raise a script execution error here?

I think reporting failure to the calling function is the way to go for unsupported properties, and raising exceptions when get/set of individual properties fails for whatever reason. The problem with that is that the calling functions were not written in a way to handle failures so revising this whole area will be a lot of work.

Comments from Reviewable

@livecodeian
Copy link
Contributor Author

Review status: 54 of 55 files reviewed at latest revision, 5 unresolved discussions.


engine/src/w32-ds-player.cpp, line 605 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Would it be better to use an explicit rounding function here?

For whatever reason the timeScale player property was defined as a positive integer, so the assumption here is that there will always be a whole number of interval units per second so no rounding required

Comments from Reviewable

@livecodeian
Copy link
Contributor Author

Review status: 54 of 55 files reviewed at latest revision, 5 unresolved discussions.


engine/src/sysdefs.h, line 53 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Could you please add some comments that explain what FEATURE_PLATFORM_APPLICATION and FEATURE_PLATFORM_WINDOW imply? Unfortunately it's not obvious from inspection.

Well, I added that so I could use the code in desktop.cpp and platform.cpp that handles the binding between MCPlatformPlayer and the MCPlayer control without having to go the whole hog and completely port the Windows engine to use MCPlatform. It might not be the best way to have done it. I'm wondering now if I should have moved the relevant code to platform-player.cpp and player-platform.cpp instead.

Comments from Reviewable

@peter-b
Copy link
Contributor

peter-b commented May 18, 2016

Review status: 54 of 55 files reviewed at latest revision, 4 unresolved discussions.


engine/src/platform.h, line 1026 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

Yup, I've swapped this out with MCGIntegerSize

OK, thanks!

engine/src/sysdefs.h, line 53 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

Well, I added that so I could use the code in desktop.cpp and platform.cpp that handles the binding between MCPlatformPlayer and the MCPlayer control without having to go the whole hog and completely port the Windows engine to use MCPlatform. It might not be the best way to have done it. I'm wondering now if I should have moved the relevant code to platform-player.cpp and player-platform.cpp instead.

Would there be any other benefits to porting the Windows engine to MCPlatform? Should I add it to the backlog?

engine/src/w32-ds-player.cpp, line 1081 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

I think reporting failure to the calling function is the way to go for unsupported properties, and raising exceptions when get/set of individual properties fails for whatever reason. The problem with that is that the calling functions were not written in a way to handle failures so revising this whole area will be a lot of work.

OK, I have added "sort out error propagation from player controls" to the backlog.

Comments from Reviewable

@livecodeian
Copy link
Contributor Author

I have now added Changes: sections to the affected docs and updated Copyright notices where appropriate.

Previously, peter-b (Peter TB Brett) wrote…

Do you think it would be a good idea to add the version in which each dictionary entry became deprecated? I often find that that's a useful piece of information to have (but that might just be me).

I also note that you've marked certain dictionary entries as no longer available on Windows without either marking them as deprecated or adding a "Changes" section.

It would be good to double-check that files you've significantly modified have their copyright headers updated to "2016".


Review status: 54 of 55 files reviewed at latest revision, 2 unresolved discussions.


engine/src/sysdefs.h, line 53 [r3] (raw file):

Previously, peter-b (Peter TB Brett) wrote…

Would there be any other benefits to porting the Windows engine to MCPlatform? Should I add it to the backlog?

You can add that to the backlog.

Comments from Reviewable

@livecodeian
Copy link
Contributor Author

Review status: 54 of 55 files reviewed at latest revision, 2 unresolved discussions.


engine/src/w32-ds-player.cpp, line 618 [r3] (raw file):

Previously, livecodeian (Ian Macphail) wrote…

I'd rather it fail than return an incorrect value.

I've added range checking to these now so overflow can be caught.

Comments from Reviewable

@peter-b
Copy link
Contributor

peter-b commented May 19, 2016

@livecodeian Thanks for addressing all those points! I think that all that's needed now is a nice comprehensive release note and this is ready to go!

Previously, livecodeian (Ian Macphail) wrote…

I have now added Changes: sections to the affected docs and updated Copyright notices where appropriate.


Reviewed 1 of 1 files at r6, 20 of 20 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@livecodeian
Copy link
Contributor Author

Release note added.

Previously, peter-b (Peter TB Brett) wrote…

@livecodeian Thanks for addressing all those points! I think that all that's needed now is a nice comprehensive release note and this is ready to go!


Review status: 55 of 56 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@peter-b
Copy link
Contributor

peter-b commented May 19, 2016

@livecode-vulcan review ok 37f1c3a

@livecode-vulcan
Copy link
Contributor

💙 review by @peter-b ok 37f1c3a

livecode-vulcan added a commit that referenced this pull request May 19, 2016
…edia

[[ WindowsPlayer ]] Update the player object on Windows to use DirectShow

Modified MCPlatformPlayer to return the native view, using MCNativeLayer to manage geometry / visibility changes.
Implemented new MCPlatformPlayer based on DirectShow - switched Windows engine to use MCPlayer based on MCPlatformPlayer.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/livecode/livecode/4022)
<!-- Reviewable:end -->
@livecode-vulcan
Copy link
Contributor

😞 test failure 37f1c3a

@peter-b
Copy link
Contributor

peter-b commented May 19, 2016

@livecodeian Oops, I wish you'd told me you'd pushed these additional changes! We could have got them merged in today! 😞

@livecode-vulcan review ok 7a33cff

@livecode-vulcan
Copy link
Contributor

💙 review by @peter-b ok 7a33cff

livecode-vulcan added a commit that referenced this pull request May 19, 2016
…edia

[[ WindowsPlayer ]] Update the player object on Windows to use DirectShow

Modified MCPlatformPlayer to return the native view, using MCNativeLayer to manage geometry / visibility changes.
Implemented new MCPlatformPlayer based on DirectShow - switched Windows engine to use MCPlayer based on MCPlatformPlayer.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/livecode/livecode/4022)
<!-- Reviewable:end -->
@livecode-vulcan
Copy link
Contributor

😎 test success 7a33cff

@peter-b peter-b merged commit dd63fa8 into livecode:develop May 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants