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

Update for LiCVPR processor.py #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update for LiCVPR processor.py #10

wants to merge 1 commit into from

Conversation

blank-ed
Copy link

No description provided.

@blank-ed blank-ed closed this by deleting the head repository Sep 26, 2022
@blank-ed blank-ed reopened this Sep 26, 2022
@SamProell
Copy link
Owner

Hi @blank-ed thank you very much for contributing. Could you please create a single PR with all changed files?
Not sure if you wanted me to review already, since the PRs are marked as drafts.

Commenting here anyways on a couple of things I spotted when going through your pull requests (PR 7-10) :

  • I see you are introducing a new dependency with tf_bodypix, which I am not against per se. Just wondering if this could also be achieved with MediaPipe's Selfie segmentation. MediaPipe is already used for Face detection so it might make sense sticking to it.
  • most of the infrastructure for getting the mean background color is already inplace. The RegionOfInterest class takes a bgmask and stores it in roi._bgmask. Also there is a background option in roi.get_mean_rgb. We only need to take a closer look at how we get the bgmask in the first place.
  • I would prefer not to alter the Processor base class but rather create a new subclass that actually uses the background information. Other processors don't need it so the change to the base might break some things..

Hope this helps. Let me know if you want to discuss something in more detail!

@blank-ed
Copy link
Author

blank-ed commented Sep 27, 2022

Hi @SamProell. I'm not really familiar with GitHub, and I tried combining them in a single PR but I couldn't do it 😂. Please let me know how to do this, thank you! Also, below the This branch has no conflicts with the base branch, it says "Only those with write access to this repository can merge pull requests". So I think you might be able to merge all the 4 PR's? Please let me know about this, thank you.

Oh yeah, sorry, I didn't know that drafts couldn't be reviewed. My bad. I will mark them ready to be reviewed.

  • Sure, I'll modify it to use the selfie segmentation method from Mediapipe. This would definitely speed up the process as well. I don't even know why I used the tf_bodypix. I was initially checking if I could obtain the background mask, as you did in your code for the ROI, and it wasn't working so I thought it was a fault with Mediapipe. Then I looked into tf_bodypix and realized it gave the same problem, and since I was already at tf_bodypix, I just used it to obtain the background mask through thresholding and etc.
  • I couldn't really place the use of bgmask, so I just left it as it is and added a new mask_bg. Sure, I will use that bgmask and remove the mask_bg.
  • Ok, sure, I was thinking of creating a subclass but I didn't really see the need for extra steps. Because LiCVPR only uses the background, I thought it would be ok to just make a condition. But yeah, that can get mixed up with some other rPPG methods in case the bg_rgb is accidentally set as True, so I will add a subclass for the background.

Your comments definitely helped. Thank you!

Also, I have a question regarding LiCVPR, especially the NLMS adaptive filter. So from the paper's implementation of the NLMS filter, it goes something like this:

image

They did not specify the initial value of the h at j=0, which is h(0) along with the initial value of gIR at j=0, which is gIR(0). As far as I understand, we need initial value of h (which is h(0)) in order to calculate the initial value of gIR(0), so that we can obtain h(1) and so on. Since they have provided the formula of how the NLMS filter is used, do we just assume our initial value of h(0) to be 0, or am I heading in the wrong direction?

@blank-ed blank-ed marked this pull request as ready for review September 27, 2022 03:22
@SamProell
Copy link
Owner

@blank-ed, I am not sure what the problem is. In your fork, you would need to create new branch, commit and push changes to all files and then create the PR from the new branch. I quickly tested this with a different account and it worked without any problems.
Once we have a single pull requests with all changes, I will review it more thoroughly and merge it into yarppg when it's ready.
It is important to have a single PR, so that we can test the final functionality after all the changes.

I added the bgmask exactly for this, but I never managed to complete the LiCVPR method. So your contribution is more than welcome.
Yes, a zero initialization should be the way to go. Although it is not explicitly mentioned in the paper. The NLMS algorithm described in wikipedia uses zeros as well.

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.

2 participants