yarppg icon indicating copy to clipboard operation
yarppg copied to clipboard

Update for LiCVPR processor.py

Open blank-ed opened this issue 3 years ago • 3 comments

blank-ed avatar Sep 25 '22 11:09 blank-ed

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!

SamProell avatar Sep 26 '22 15:09 SamProell

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 avatar Sep 27 '22 03:09 blank-ed

@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.

SamProell avatar Sep 28 '22 16:09 SamProell