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

[CHG] added ShowPropertyPage method to DirectShowDevice class #110

Closed
wants to merge 1 commit into from
Closed

[CHG] added ShowPropertyPage method to DirectShowDevice class #110

wants to merge 1 commit into from

Conversation

flat-eric147
Copy link

No description provided.

@flat-eric147
Copy link
Author

Hi, I had the necessity to show the native dialog of the DirectShow source filter in the UI, see also:

https://learn.microsoft.com/en-us/windows/win32/directshow/displaying-a-filters-property-pages
showdialog

Some vendors (like here Blackmagic WDM Capture filter) offer some important settings in those property pages:
snapBMProps

I also extended the WPF sample with some buttons and a dropdown to select the source from:
flashcapWPF

Not sure if this can be merged back to the main repository in terms of coding standard :)

@kekyo
Copy link
Owner

kekyo commented Nov 10, 2023

@flat-eric147 Can I separate and merge only the modification of the transcoder option?


The current implementation understands that transcoder is always Auto by the following line:

                            BitmapTranscoder.Transcode(
                                pBih->biWidth, pBih->biHeight,
                                pBih->biCompression, YUV2RGBConversionStandard.Auto, true,    // Always `Auto`
                                pImageContainer + pBfh->bfOffBits,
                                pTranscodedImageContainer + pBfhTo->bfOffBits);

I think we need to make minor modifications in various places to get the information from here to the public interface transcodeIfYUV flag. This is being tested in my trial implementation (a0a894f). In the meantime, I will fill in the last piece after I merge your results.

@flat-eric147
Copy link
Author

@flat-eric147 Can I separate and merge only the modification of the transcoder option?

The current implementation understands that transcoder is always Auto by the following line:

                            BitmapTranscoder.Transcode(
                                pBih->biWidth, pBih->biHeight,
                                pBih->biCompression, YUV2RGBConversionStandard.Auto, true,    // Always `Auto`
                                pImageContainer + pBfh->bfOffBits,
                                pTranscodedImageContainer + pBfhTo->bfOffBits);

I think we need to make minor modifications in various places to get the information from here to the public interface transcodeIfYUV flag. This is being tested in my trial implementation (a0a894f). In the meantime, I will fill in the last piece after I merge your results.

Yes, it makes sense to expose it to the public interface, but I prefer to leave that work to you as you know better than me what to do and where to do it. I'm also unsure about the performFullRange parameter which was unused before. Please merge whatever you think makes sense, I'm happy with anything you merge back. Let me know if you need anything do to from my side. Thank you!

@kekyo kekyo mentioned this pull request Nov 11, 2023
@kekyo
Copy link
Owner

kekyo commented Nov 11, 2023

@flat-eric147 Cherry-picked your YUV transcode fragment and merged extended interface into 295de15 . Please verify it and modify when a desirable pattern can be considered.

  • Option enum type renamed to TranscodeFormats .
  • In TranscodeFromYUVInternal():
    • Could it be that the logical decision for each full range/limited range is the other way around? (good if not a problem)
    • BT.609 did not have one of the range checking, so the enumeration type is not extended either (BT609FullRange is not defined).

@flat-eric147
Copy link
Author

transcode.patch

Hi, I attach a patch since I cannot push to the original repository and I don't know how to sync the new branch into my forked repository.

  • Could it be that the logical decision for each full range/limited range is the other way around? (good if not a problem)

yes, I am unsure how to put that logic, feel free to change it!

  • BT.609 did not have one of the range checking, so the enumeration type is not extended either (BT609FullRange is not defined).

Just added it. It was there implicitly with the default initial values of the multiplier variables. Made it more explicit and readable now.

@kekyo
Copy link
Owner

kekyo commented Nov 12, 2023

Hi, I attach a patch since I cannot push to the original repository and I don't know how to sync the new branch into my forked repository.

You mean you don't know how to make things work with git operation? There are many ways to handle the problem in Git, and this is one of them (this is how I would operate).

  • Your develop branch is used directly in PR, so you manipulate it.
  1. git checkout develop to check out the local develop branch.
  2. git reset --hard 53cf67a32a6a41c48be293c6238c08d967d9d85e to force a change develop branch position to [CHG] added ...
    • Commits after that point have already been merged into FlashCap's develop branch.
  3. git rebase flashcap/develop to rebase into FlashCap's develop branch. Here flashcap is the remote name of the FlashCap repository, so use the name you gave there.
  4. git push -f to force push develop branch to your repository.

Now you should have 53cf67a32a6a6a41c48be293c6238c08d967d9d85e changes committed to the latest location (commit ID will change).

After that, you should be able to commit the patch.

If you can't get it to work, let me know and I'll do it here. (I try to leave modifications to the person who wrote the code if there is no reason to do so, since the code belongs to that person ;)

@flat-eric147
Copy link
Author

Appreciate the instructions but I failed miserably :) Especially the rebase command did not work, I tried a lot of variations but I always get "invalid upstream". Please use my patch, I don't mind not getting any credit for this ;)

@kekyo
Copy link
Owner

kekyo commented Nov 13, 2023

No, problem! I will make it.

@kekyo
Copy link
Owner

kekyo commented Nov 13, 2023

Moved #116.

@kekyo kekyo closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Added ShowPropertyPage method to DirectShowDevice class
2 participants