Closed Bug 905650 Opened 11 years ago Closed 8 years ago

Add a way to get just the hash of a screenshot

Categories

(Testing :: Marionette Client and Harness, defect)

defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: jgraham, Assigned: kb15, Mentored)

Details

(Keywords: pi-marionette-client, pi-marionette-server, Whiteboard: [screenshot][good first bug][lang=py][lang=js])

Attachments

(1 file)

For many use cases it isn't necessary to get all the screenshot data; you just want to compare the screenshot against some known reference. One obvious example of this is implementing reftests where you only care about whether two screenshots are the same or different, not about the details of the individual pixel data.

This could be implemented by allowing a hash of the screenshot to be returned by the screenShot() function in marionette-listener.js, and adding a parameter indicating that only the hash is required, or by having a separate function that does largely the same thing as screenShot, but returns a hash.

One use case that could be common is "match the hash to an existing one and only return the screenshot data if there is a mismatch". This would be useful for testing scenarios where the screenshot data is debugging information for the case that hashes fail to match. For full generality, one should be able to return only on match or only on mismatch (this is the reftest usecase with == and != reftests).
cool idea
Whiteboard: [screenshot]
Having it as a separate command alleviates any typing problems in static languages.  Otherwise you need to look at the parameter the user gave it to know what the return type will be.  Because JSON doesn't distinguish between a base64 string of a PNG and a stringified byte array of a hash, it would be impossible to distinguish based on the JSON return type.
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [screenshot] → [screenshot][good first bug] [mentor=ato][lang=py][lang=js]
Whiteboard: [screenshot][good first bug] [mentor=ato][lang=py][lang=js] → [screenshot][good first bug][lang=py][lang=js]
import difflib

def diff_ratio(screen_ref, screen_new):
    s = difflib.SequenceMatcher(None, screen_ref, screen_new)
    return s.quick_ratio()


This will return a parameter in between 0 and 1.
If the two images are identical, you get a 1, if it's totally different you get a 0.
I think what is suggested here is to add this as a server-side command to reduce wire protocol delays.
I would like to take this bug.
(In reply to Chris Galecki from comment #5)
> I would like to take this bug.

Assigned the bug to you.  Please let me know if you need any help.  I’m known as ato in the #ateam IRC channel.
Assignee: nobody → csg35
Hi Andreas

I am interested in working on this. Can you please mentor me?

Thanks 
Dhruva
Reassigning this to es13b1022 since there has been no activity.

I’m happy to mentor.  The source code for the existing screenshot feature is in testing/marionette/driver.js and testing/marionette/listener.js.  All remote calls go to the driver handler that lives in chrome space (driver.js) and gets propagated to the listener (listener.js) if a content screenshot is desired.  The result is returned to chrome context, then marshaled and passed back to the local end.

What I imagine we want to do here is to add a separate command endpoint that returns a hash of the screenshot raw data.  We could abstract the code in the existing screenshot implementation into a function that can get called by multiple different users.
Assignee: csg35 → es13b1022
Status: NEW → ASSIGNED
Hi, Are you still working on this or can I reassign this back in case anyone else wants to work on it?
Flags: needinfo?(es13b1022)
Resetting assignee
Assignee: es13b1022 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(es13b1022)
I would like to work on this bug can you please assign this bug to me 

also I will try to sent the patch soon and is there anything else that I need to know to fix this bug ?
Flags: needinfo?(james)
I don't think there's anything else you need to know; see the comments from :ato above, and ask on irc when you get stuck.
Assignee: nobody → dhanvicse
Flags: needinfo?(james)
Since last time I commented on this bug, I've refactored the screen capture code as mentioned above to be possible to share between chrome- and content space.

The refactor actually makes it easier to fix this bug as the functions to capture screenshots are now more compartmentalised and not so tightly coupled as before.

See the source code in testing/marionette/capture.js: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/capture.js?from=marionette%2Fcapture.js
Status: NEW → ASSIGNED
Assignee: dhanvicse → nobody
Status: ASSIGNED → NEW
Mentor: ato
Assignee: nobody → dhanvicse
Status: NEW → ASSIGNED
I would like to still work on this bug so I am assigning it to myself
Any progress on this?  Can I help in any way?
Flags: needinfo?(dhanvicse)
I am afraid that I made a little progress on this one 

Yes I sure need your help, I will ping you in IRC
Flags: needinfo?(dhanvicse)
Attachment #8739608 - Flags: review?(ato) → review+
Comment on attachment 8739608 [details]
MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato

https://reviewboard.mozilla.org/r/45117/#review41859

This looks very good!  My comments are almost entirely nitpicks and I’m giving this an r+.

Do you have access to test code on try or push to inbound?  If not we can get the ball rolling on that.

::: testing/marionette/capture.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +var {utils: Cu} = Components;

`const`

::: testing/marionette/capture.js:158
(Diff revision 1)
> +* @return {string}
> +*     A hex digest of the SHA-256 hash of the base64 encoded string.
> +*/
> +capture.toHash = function(canvas) {
> +  let u = capture.toBase64(canvas);
> +  var buffer = new TextEncoder("utf-8").encode(u);

`let`

::: testing/marionette/capture.js:159
(Diff revision 1)
> +  return crypto.subtle.digest("SHA-256", buffer).then(function (hash){
> +    return hex(hash);
> +  });

This can be shortened using ES6 arrow functions, but you can decide if you want to use it:

```js
return crypto.subtle.digest("SHA-256", buffer).then(hash => hex(hash));
```

::: testing/marionette/capture.js:162
(Diff revision 1)
> +  let u = capture.toBase64(canvas);
> +  var buffer = new TextEncoder("utf-8").encode(u);
> +  return crypto.subtle.digest("SHA-256", buffer).then(function (hash){
> +    return hex(hash);
> +  });
> +}

Missing semicolon

::: testing/marionette/capture.js:163
(Diff revision 1)
> +  var buffer = new TextEncoder("utf-8").encode(u);
> +  return crypto.subtle.digest("SHA-256", buffer).then(function (hash){
> +    return hex(hash);
> +  });
> +}
> + 

Whitespace

::: testing/marionette/capture.js:174
(Diff revision 1)
> +  var hexCodes = [];
> +  var view = new DataView(buffer);
> +  for (var i = 0; i < view.byteLength; i += 4) {
> +    var value = view.getUint32(i)
> +    var stringValue = value.toString(16)
> +    var padding = '00000000'
> +    var paddedValue = (padding + stringValue).slice(-padding.length)

Use `let` throughout.

::: testing/marionette/capture.js:177
(Diff revision 1)
> +    var value = view.getUint32(i)
> +    var stringValue = value.toString(16)
> +    var padding = '00000000'
> +    var paddedValue = (padding + stringValue).slice(-padding.length)

Missing semicolons

::: testing/marionette/driver.js:2514
(Diff revision 1)
>   * @return {string}
> - *     PNG image encoded as base64 encoded string.
> + *     If 'hash' is False, PNG image encoded as base64 encoded string. If
> + *     'hash' is True, hex digest of the SHA-256 hash of the base64 encoded
> + *     string.

'hash' → {@code hash}
False → false

::: testing/marionette/driver.js:2564
(Diff revision 1)
>        let dataUrl = canvas.toDataURL("image/png", "");
>        let data = dataUrl.substring(dataUrl.indexOf(",") + 1);
>        resp.body.value = data;
>        break;
>  
> -    case Context.CONTENT:
> +    case Context.CONTENT:      

Whitespace

::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:5
(Diff revision 1)
>  import base64
>  import imghdr
>  import struct
>  import urllib
> +import hashlib 

Sort alphabetically

::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:9
(Diff revision 1)
>  
>  import base64
>  import imghdr
>  import struct
>  import urllib
> +import hashlib 

Whitespace

::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:200
(Diff revision 1)
> +
> +    def test_hash_format(self):
> +        self.marionette.navigate(box)
> +        el = self.marionette.find_element(By.TAG_NAME, "div")
> +        content = self.marionette.screenshot(element=el, format="hash")
> +        pyHash = hashlib.sha256(ELEMENT).hexdigest()

pyHash → hash

::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:201
(Diff revision 1)
> +    def test_hash_format(self):
> +        self.marionette.navigate(box)
> +        el = self.marionette.find_element(By.TAG_NAME, "div")
> +        content = self.marionette.screenshot(element=el, format="hash")
> +        pyHash = hashlib.sha256(ELEMENT).hexdigest()
> +        self.assertEqual(content, pyHash) 

Whitespace

::: testing/marionette/listener.js:1756
(Diff revision 1)
>   *     references.
>   *
>   * @return {string}
>   *     Base64 encoded string of an image/png type.
>   */
> +

Remove blank line

::: testing/marionette/listener.js:1800
(Diff revision 1)
> +*     references.
> +*
> +* @return {HTMLCanvasElement}
> +*     The canvas element to be encoded or hashed.
> +*/
> +

Remove blank line
Assignee: dhanvicse → kbmoz1515
I don't have access to try or push to inbound.

Thanks for the quick review; I'll get to work on these changes!
Flags: needinfo?(ato)
Flags: needinfo?(ato)
Comment on attachment 8739608 [details]
MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45117/diff/1-2/
https://reviewboard.mozilla.org/r/45117/#review44489

Sorry for the late reply to this, but I’ve been on holiday!  This looks excellent!  Thank you so much for fixing this bug.  I will run a try run (CI) tomorrow (unless you have access to do so already?), and if all is fine push it to inbound.
I notice that the patches have not be rebased for quite some time.  The test that is failing in the try run was disabled by philor in bug 1267012 and should be of no concern.

kb15: Can you rebase the patch and we’ll do another try run?
Flags: needinfo?(kbmoz1515)
Comment on attachment 8739608 [details]
MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45117/diff/2-3/
I pulled inbound today and rebased the patch.
Flags: needinfo?(kbmoz1515)
Thanks! Triggered another try run.
One failing Mn-e10s job, but it might be an intermittent so retriggering.
The failing Mn-e10s job was indeed intermittent.  I’ve landed it on inbound now.  That means it will hopefully pass all tests and make its way to central within 24 hours.
I forgot to mention: Thank you for fixing!
https://hg.mozilla.org/mozilla-central/rev/69267d635ade
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.