Google Cloud Vision API offers yet another feature- Facial Detection in an image. Not only the location of the face in the image, but also the emotion of the person in terms of four expressions- joy, sorrow, anger and surprise.
Emotions in the image can be really important when a person is uploading his image as their profile picture for professional purposes.

Hence, I would implement this feature to implement Emotion Detection with the use case that if the person uploads his profile picture with no happy expressions, he would be notified of his expressions, and would be requested to change the image to a happy one.
In short, try to make sure that the person is happy in their profile pictures.

Remaining Tasks
Develop the patch for this issue.

(This is one of the tasks of my GSOC 2016 project.)

CommentFileSizeAuthor
#41 interdiff-38-41.txt11.58 KBajalan065
#41 Base-class-for-test-functions-2759031-8-41.patch9.08 KBajalan065
#38 interdiff-31-38.txt4.38 KBajalan065
#38 Property-added-and-variables-changed-2759031-8-38.patch8.55 KBajalan065
#31 Test-for-emotion-detection-2759031-8-31.patch8.1 KBajalan065
#28 set-message-replaced-with-addviolation-2759031-8-28.patch762 bytesajalan065
#21 interdiff-17-21.txt419 bytesajalan065
#21 Schema-updated-2759031-8-21.patch5.63 KBajalan065
#17 Emotion-detection-on-fields-2759031-8-17.patch5.22 KBajalan065
#5 Passed-user-object-2759031-8-5.patch2.23 KBajalan065
#3 Emotion-detection-2759031-8-3.patch2.14 KBajalan065

Comments

ajalan065 created an issue. See original summary.

ajalan065’s picture

ajalan065 commented
Issue summary: View changes
ajalan065’s picture

ajalan065 commented
Status: Active » Needs review
StatusFileSize
new Emotion-detection-2759031-8-3.patch2.14 KB

I had tried to put the detection feature on the user profile pictures.
However, the control is not validating the line 

if(!empty($user->get($key)->target_id)) { ..

and skipping the if statement.
Also, the line 

$file_uri = $user->get($key)->entity->getFileUri();

does not work.

Where did I do wrong? Please review.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work 
+++ b/google_vision.module
@@ -155,3 +160,29 @@ function google_vision_file_entity_add_labels($file, $field, $vid) {
+  $user = 'user';
...
+      if(!empty($user->get($key)->target_id)) {

$user is not an object. Please pass the entity as an argument.

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review
StatusFileSize
new Passed-user-object-2759031-8-5.patch2.23 KB

Passed the entity as the argument to the function.
But still the code does not execute.
I tried a lot debugging it, but could not. I am not able to get where is the code breaking.
Please guide me.

ajalan065’s picture

ajalan065 commented

A point regarding the above patch.
Its not that the patch does not work at all.
The drupal message regarding the emotion appears when the user clicks on 'Create Account' button.

eugene.ilyin’s picture

eugene.ilyin commented
Status: Needs review » Needs work

It works for me and I see the message "You seem to be unhappy".

What I think:

1. Better to check the image and show the warning only if the file was changed. So, I propose to add the setting for the field and execute this checking when the file will be updated. It can help to reduce the count of requests to Google API.

2. I'm not sure that text of the warning message is good. Maybe should we add proposition, like - please upload a photo where you are smiling and happy?

3. It's just warning but not the validation. So, I think that we should check that current request executed through web but not through cli. Because this warning doesn't make sense for the case with cli.

What do you think about my remarks?

ajalan065’s picture

ajalan065 commented

@eugene.ilyin,
I could not understand what you want to state by point 1. Can you please clarify it?

In point 3, if we add it as a constraint and validate it, then it might be pretty annoying for the user who is registering; as in that case, it would be a compulsion to be happy on their profile pictures. I have developed the patch on constraints also. So, if you still convinced that we should put it as a constraint, then I will post it.

eugene.ilyin’s picture

eugene.ilyin commented

I could not understand what you want to state by point 1. Can you please clarify it?

I propose to check not the user entity but file entity. To be able to check is face happy or no for any field. Could you add the setting for this option like with safe search?

So, if you still convinced that we should put it as a constraint, then I will post it.

No, let's leave it just as the warning. But if the developer/user will do the mass import of entities through feeds for example, better to do not check images if they are imported through CLI. If it will be the case with WEB, so seems we cannot handle it correctly and let's don't worry about it.
Do you understand what I mean?

ajalan065’s picture

ajalan065 commented

So, you mean that face detection should be implemented exactly the same way as the safe search?
And the site administrator would have the control to keep the face detection check at any image field and not only the user picture?

Did I get your point correctly?

eugene.ilyin’s picture

eugene.ilyin commented

Yes, I think it's more flexible and preferable. Not good to have it by default. Do you agree with me?

ajalan065’s picture

ajalan065 commented

Ya. That may be better to have it an option. I would post the patch by today.

ajalan065’s picture

ajalan065 commented

@eugene.ilyin,
I tried to work that out. But the problem is checking the file entity would not be of any good, I believe. Because, the option of Emotion Detection is present on the image fields and not the Image file entity.

So, shall I continue with putting the check on user entity itself? I have added the option to the image fields, thus reducing the API requests.

eugene.ilyin’s picture

eugene.ilyin commented

But the problem is checking the file entity would not be of any good, I believe. Because, the option of Emotion Detection is present on the image fields and not the Image file entity.

Only if you will enable it. It's configuration. I think it's nice when you can enable or disable something. It's flexible. What the problem?

ajalan065’s picture

ajalan065 commented

Ya, I understood that it will work only when it is enabled. That has been done.
However, the configuration is a part of image field, just as the Safe search, and not the file entity (like Enable Google Vision configuration).
So, if we check the file entity, we would not be able to get the field and its definitions for the particular image field.
What I believe is, instead of checking the file, we should fetch the value from the image field, which requires the checking of user/node entities, just as we had done in safe search.

Here lies the problem of checking the file entity.

But, if we check the user entity, we can easily solve the problem.

Another approach may be the use of constraints and validators just as we had done for safe search. If we apply constraints, there is no need to check any entity. The constraint may directly be applied to the fields.

So, what you suggest? Use of constraints? Or checking the user entities?

ajalan065’s picture

ajalan065 commented

Moreover, there is not much use of emotion detection, except in the profile pictures. This is just my thinking :)

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review
StatusFileSize
new Emotion-detection-on-fields-2759031-8-17.patch5.22 KB

Here is the patch which works as suggested in #7.
It provides a configuration to not only user entities but to all the image fields as suggested.
It works in similar way as the safe search constraint works, however, with a difference.
Unlike the safe search which puts a constraint on the fields, it just gives a message if the emotion violates the constraint. The emotion is not put as a binding constraint, as it can be pretty annoying in case of creation of new accounts(#8).

Hope this solves our purpose :)

Status: Needs review » Needs work

The last submitted patch, 17: Emotion-detection-on-fields-2759031-8-17.patch, failed testing.

The last submitted patch, 17: Emotion-detection-on-fields-2759031-8-17.patch, failed testing.

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review

The test for safe search constraint validation fails, which I suppose is not so important here.
Hence, setting it back to "Needs Review"

ajalan065’s picture

ajalan065 commented
StatusFileSize
new Schema-updated-2759031-8-21.patch5.63 KB
new interdiff-17-21.txt419 bytes

Here is the patch with updated schema. The tests passes now.

Please review it and suggest if any changes required. Else we can move to build the tests for it.

  • ajalan065 authored ce0688d on 8.x-1.x 
    Issue #2759031 by ajalan065, eugene.ilyin: Implement the Face Detection...
eugene.ilyin’s picture

eugene.ilyin commented
Status: Needs review » Needs work

No bad, but there is a bug. If you will remove the existing image, the validation will be executed and you will see warning message.
But you should not check the image when you are removing it.

ajalan065’s picture

ajalan065 commented

The bug arises due to the use of drupal_set_message(). Instead, if we use the addViolation(), the bug is removed.
So do you suggest the use of addViolation()? In that case, it would be a compulsion to provide happy image if the configuration is on.

eugene.ilyin’s picture

eugene.ilyin commented

I'm not sure but I think that root of the bug is that when we remove the file from the field, you are checking it. It's incorrect behavior in our case. Is there a way to detect when you are removing but not uploading the file?

ajalan065’s picture

ajalan065 commented

As per me, I think that the request is made whenever any file operation is done, because both the conditions of target_id!=NULL and the file uri matches, whenever a file is uploaded or removed.
I have checked it with addViolation(). Everything works as expected.
I have not been able to come up with another way though up till now.

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review

Is there a way to detect when you are removing but not uploading the file?

I am unable to find how to check this. Need help!!

ajalan065’s picture

ajalan065 commented
StatusFileSize
new set-message-replaced-with-addviolation-2759031-8-28.patch762 bytes

The only way I found to avoid the bug.

eugene.ilyin’s picture

eugene.ilyin commented
Status: Needs review » Needs work

It's a wrong way.
I guess that you can try to check here
$data->parent->entity->values['user_picture']['x-default'][0]

let me know does it help or not?

ajalan065’s picture

ajalan065 commented

Hi eugene.ilyin,
Unfortunately, $data->parent->entity->values['user_picture']['x-default'][0] does not work :(

Can you provide with some other idea?
I am also trying to find out a way, but have not been able to till now.

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review
StatusFileSize
new Test-for-emotion-detection-2759031-8-31.patch8.1 KB

Here is the test for the patch #21

Please review :)

eugene.ilyin’s picture

eugene.ilyin commented

Tests are good, but what about solving the problem which I've described? Do you mean that you don't know how to solve it?

ajalan065’s picture

ajalan065 commented

I am trying to find the solution to the above bug you had mentioned. But not able to find it yet. Just to keep the work moving, I developed the tests, as they are irrespective of the constraint validator.
Naveen had said he would look into it.

eugene.ilyin’s picture

eugene.ilyin commented

Okay, let's wait for his opinion.

naveenvalecha’s picture

naveenvalecha
He/Him
commented

here's quick overview I'll test the functionality manually of this patch later by monday.

  1. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,226 @@
+   * @var object

    This should be
    @var \Drupal\user\UserInterface 

  2. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,226 @@
+    $form_display = \Drupal::entityTypeManager()

    Instead of loading entityTypeManager multiple times.
    Add a new protected property $entityTypeManger and pass its value in setup function.

    In Setup() it would be like
    $this->entityTypeManger = \Drupal::entityTypeManger(); 

  3. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,226 @@
+    $this->drupalGet("admin/config/people/accounts/fields/$field_id");

    use single quotes and move the variable out of double quotes. 

  4. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,226 @@
+    $user_name = $this->createUserWithProfilePicture();

    change $user_name to $username

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Needs work

correcting issue status.
@arpit,
any update on this ?

ajalan065’s picture

ajalan065 commented

Hi naveenvalecha,
I will post the new patch by EOD in accordance to #35.

However, I am still stuck with the comment #27. Need help on that.

ajalan065’s picture

ajalan065 commented
Assigned: ajalan065 » naveenvalecha
Status: Needs work » Needs review
StatusFileSize
new Property-added-and-variables-changed-2759031-8-38.patch8.55 KB
new interdiff-31-38.txt4.38 KB

Here is the patch following the suggestions of #35 by naveenvalecha. Assigning him the issue for review.

However, #27 still remains unsolved. Need your help here to solve this.

Please review :)

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Needs work 
  1. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,239 @@
+    $this->entityTypeManager = \Drupal::entityTypeManager();

    This is better than calling static method
    $this->container->->get('entity_type.manager'); 

  2. +++ b/src/Tests/UserEmotionTest.php
@@ -0,0 +1,239 @@
+  /**
+   * Creates a user with profile picture attached.
+   *
+   * @return string $edit['name'].
+   *  The user name of the newly created user.
+   */
+  public function createUserWithProfilePicture() {
+    //Get an image.
+    $images = $this->drupalGetTestFiles('image');
+    $user = 'user1';
+    $pass = 'password1';
+    $edit = [
+      'mail' => 'user@user.com',
+      'name' => $user,
+      'pass[pass1]' => $pass,
+      'pass[pass2]' => $pass,
+      'files[images_0]' => \Drupal::service('file_system')->realpath($images[0]->uri),
+    ];
+    $this->drupalPostForm(Url::fromRoute('user.admin_create'), $edit, t('Create new account'));
+    $re_edit = [
+      'pass[pass1]' => $pass,
+      'pass[pass2]' => $pass,
+      'images[0][alt]' => $this->randomMachineName(),
+    ];
+    $this->drupalPostForm(NULL, $re_edit, t('Create new account'));
+
+    return $edit['name'];
+  }

    We should create a abstract parent class and move this function to it.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Assigned: naveenvalecha » eugene.ilyin

Assigning to eugene to help you with #27

ajalan065’s picture

ajalan065 commented
Status: Needs work » Needs review
StatusFileSize
new Base-class-for-test-functions-2759031-8-41.patch9.08 KB
new interdiff-38-41.txt11.58 KB

Here is the patch following the suggestions in #39.

Please review :)

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Reviewed & tested by the community

looks good

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Assigned: eugene.ilyin » Unassigned
Status: Reviewed & tested by the community » Fixed

Patch committed and pushed to 8.x-1.x
Do file a followup for #27 and collaborate with Eugene about the same.

eugene.ilyin’s picture

eugene.ilyin commented
Status: Fixed » Needs work

@Ajalan, please create a separate issue and describe the problem. We cannot just close it and forget about the problem.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs work » Fixed

Filed a followup for the same https://www.drupal.org/node/2783941

Closing this one.
Thanks Eugene! for your concerns.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.