Closed Bug 878003 Opened 11 years ago Closed 10 years ago

Add mime type argument to getScreenshot API to control transparency of resulting screenshot

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.3 C2/1.4 S2(17jan)

People

(Reporter: alive, Assigned: timdream)

References

Details

Attachments

(3 files, 8 obsolete files)

Patch for commit (Part I)
6.18 KB, patch
timdream
: review+
Details | Diff | Splinter Review
Patch for commit (Part II)
6.91 KB, patch
timdream
: review+
Details | Diff | Splinter Review
Patch for commit (Part III)
5.21 KB, patch
timdream
: review+
Details | Diff | Splinter Review 
Currently the mozbrowser API: getScreenshot generate a JPG image thus it's not transparent.
But in some case(like homescreen) we may need a PNG version to avoid showing a gray background in the screenshot image.
Blocks: 880572
Attached patch WIP patch (obsolete) — — Splinter Review 
WIP v1(not tested)
Unfortunately the patch doesn't work.
It always gives me a black-background homescreen screenshot. Strange.
Deassign myself first.
Assignee: alive → nobody
blocking-b2g: --- → leo?
Triage - blocking as it blocks a leo+ blocker bug 880572. Whether or not 880572 is a real blocker should be discussed again with partner as it is an attempt to fix bug 871919 more correctly.
blocking-b2g: leo? → leo+
Gonna give it a try when I have the time to do so. Feel free to steal it.
Assignee: nobody → timdream
No longer blocks: browser-api
Attachment #760268 - Attachment is obsolete: true
bug 880572 is no longer a blocker.

Please renom if this alone is considered blocking.
blocking-b2g: leo+ → ---
Attached patch transparent.patch (obsolete) — — Splinter Review 
Kanru, are you eligible of reviewing mozbrowser API patch?

This is a follow-up to Alive's work -- I somehow came across the |mozOpaque| attribute we need to escape here. I tested locally and this works.
Attachment #761902 - Attachment is obsolete: true
Attachment #8342938 - Flags: review?(kchen)
Comment on attachment 8342938 [details] [diff] [review]
transparent.patch

Review of attachment 8342938 [details] [diff] [review]:
-----------------------------------------------------------------

See also https://bugzilla.mozilla.org/show_bug.cgi?id=801676 to understand why the screenshot was in JPEG format.

Could we test it the screenshot is actually transparent? Currently the getScreenshot test does not verify the captured image though..

::: dom/browser-element/BrowserElementChildPreload.js
@@ +721,5 @@
>      // - Crop the viewport so its width is no larger than maxWidth and its
>      //   height is no larger than maxHeight.
>      //
> +    // - Transparent tells us to output a PNG or JPG.
> +    //

I'm not sure if it's a good idea to mention this. The image format could be anything capable encoding the alpha channel.
Attachment #8342938 - Flags: review?(kchen) → review?(bugs)
Comment on attachment 8342938 [details] [diff] [review]
transparent.patch

What if we had mimetype as paramater? (image/jpeg or image/png)
Then we can in the future add more types, of which some may support
transparency? Also, this way it becomes clear what kind of data will be generated.
Attachment #8342938 - Flags: review?(bugs) → review-
Flags: needinfo?(timdream)
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8342938 [details] [diff] [review]
> transparent.patch
> 
> What if we had mimetype as paramater? (image/jpeg or image/png)
> Then we can in the future add more types, of which some may support
> transparency? Also, this way it becomes clear what kind of data will be
> generated.

Thanks for the review! 

If we are using mimetype here, IMHO we should expose the third argument too (quality of JPEG). What do you think?

Also, even though toBlob() is default to image/png, I would still in favor of using JPEG for this method by default since I was told by :jlebar that results lower memory footprints. Besides, changing API default will make this Gecko bug blocked by another Gaia bug and force us to do yet another gaia/gecko repo sync landing dance, something I would like to avoid. Does that work for you too?
Flags: needinfo?(timdream) → needinfo?(bugs)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> If we are using mimetype here, IMHO we should expose the third argument too
> (quality of JPEG). What do you think?
That could be added later if needed, right?

> Also, even though toBlob() is default to image/png, I would still in favor
> of using JPEG for this method by default since I was told by :jlebar that
> results lower memory footprints. Besides, changing API default will make
> this Gecko bug blocked by another Gaia bug and force us to do yet another
> gaia/gecko repo sync landing dance, something I would like to avoid. Does
> that work for you too?
Yeah, defaulting to jpeg if not mimetype is passed sounds still ok.
It is a bit inconsistent with toBlob but taking a screenshot is quite different anyway.
Flags: needinfo?(bugs)
Attached patch WIP v2 (obsolete) — — Splinter Review 

        Attachment #8342938 -
        Attachment is obsolete: true

        Attachment #8345200 -
        Flags: review?(bugs)
Summary: Add third argument to getScreenshot API: transparent or not → Add mime type argument to getScreenshot API to control transparency of resulting screenshot

    
    

    
  
I have test almost ready and will push to try tomorrow.

        Attachment #8345200 -
        Flags: review?(bugs) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug878003-test.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I will set review after verifying the test on try and/or locally.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug878003-test.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I modify the test and try not to change it's exec flow (which is a bit confusing to be honest)

-- The first screenshot will now called with image/jpeg
-- The second screenshot will now called with image/png
-- when the screenshots are received, the image will be passed to a canvas and we will read the 4th value of the image data array (the alpha channel of the first pixel) to see if the image is indeed opaque/transparent.

I have verified the test passes locally. Took me some time to figure out how to run mochitest locally (I tried to run it with B2G/Desktop and I eventually give up and build Firefox Desktop).

Try server submission:
https://tbpl.mozilla.org/?tree=Try&rev=f25e37636140

        Attachment #8345765 -
        Flags: review?(bugs)

        Attachment #8345674 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8345765 [details] [diff] [review]
bug878003-test.patch

try failed :-/

        Attachment #8345765 -
        Flags: review?(bugs)

    
    

    
  
OK, I didn't look into the tests closely enough. What passes locally was the inproc test. For oop test it will fail on my machine as well. Strangely, I could visually confirm the screenshots are indeed transparent on the phone even if all the app frames are oop. 

Could there be something different between oop'd frame in Fx Desktop and B2G? I am going to run the tests (inproc and oop) with emulator to see if it failed on B2G too first.

    
    

    
  
I am a little confused by TryServer coz it looks like emulator run doesn't cover dom/ tests?
https://tbpl.mozilla.org/?tree=Try&rev=9a8fafd373ad

linux64 build do, however:
https://tbpl.mozilla.org/?tree=Try&rev=5acd05b2ccde

    
    

    
  
After consulting with Kanru and appendChild(img) to see what's really going on, we found that on Firefox desktop, an oop frame in Firefox desktop will always painted with an opaque solid color -- the default background color of user's choice -- browser.display.background_color. Attempt to set the background color to transparent will not work -- something we need to find a fix or it will forever block testing on Desktop. :mattwoodrow, Kanru told me you might have some idea on this, could you help out?

Also, it is appeared that emulator does not run dom/browser-elements/ tests at all. I checked fulllog of 1-9. One idea of mime is to figure out how to make TBPL only run this oop tests in emulator but not other tests, and skip this expected failed test in Desktop builds. Need to figure out how to do it. But first I would like to see if :smaug thinks this is a good idea.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> Also, it is appeared that emulator does not run dom/browser-elements/ tests
> at all. I checked fulllog of 1-9. One idea of mime is to figure out how to
> make TBPL only run this oop tests in emulator but not other tests, and skip
> this expected failed test in Desktop builds. Need to figure out how to do
> it. But first I would like to see if :smaug thinks this is a good idea.

BTW I still need to make sure the oop test passes on emulator first.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> Also, it is appeared that emulator does not run dom/browser-elements/ tests
> at all. I checked fulllog of 1-9. One idea of mime is to figure out how to
> make TBPL only run this oop tests in emulator but not other tests, and skip
> this expected failed test in Desktop builds. Need to figure out how to do
> it. But first I would like to see if :smaug thinks this is a good idea.

Perhaps don't skip but mark the test expected fail.
Flags: needinfo?(bugs)

    
    

    
  
Thanks for the reply.

So I did some digging on tbpl again and I found the mochitest numbers have changed since. We no longer run browser_element tests on Desktop Firefox (these builds run "M (1 2 3 4 5 bc oth)" and no test results show up in any of the full logs.

Emulator now shows "M (1 2 3 4 5 6 7 8 9) and browser_element tests did pop-up on mochitest-4.

Try is currently closed. I have rebase'd my local tree and will push a "try: -b o -p emulator -u mochitest-4 -t none" try again and see if the original test passes. If so I think the tests can be checked-in w/o being marked as expected failure.

    
    

    
  
Ouch, I found this:

https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json#284

hg blame showed emulator was NEVER configured to run mozbrowser tests when it was being enabled in TPBL. I will push a try to enable everything and see what will happen.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #25)
> hg blame showed emulator was NEVER configured to run mozbrowser tests when
> it was being enabled in TPBL. I will push a try to enable everything and see
> what will happen.

Bug 830523

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=2c80ede259b6

try: -b o -p emulator -u mochitest-1,mochitest-2,mochitest-3,mochitest-4,mochitest-5,mochitest-6,mochitest-7,mochitest-8 -t none

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=0d2f5c4e9730

try: -b o -p linux64,macosx64,win32 -u mochitest-2 -t none

    
    

    
  
Verified browser-elements tests are being run on m-3 for emulator (comment 27). Gonna to disable the failed tests and push again. oop tests are not being run; need to investigate why.

The inproc test on Android passes in comment 28 (oop is not tested).

There was an logic error on my patch on |if (!isB2G ...| so test failed on comment 29 (desktop builds), so re-push to try here:

https://tbpl.mozilla.org/?tree=Try&rev=17f8462f1aef

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #30)
> The inproc test on Android passes in comment 28 (oop is not tested).

It looks like we disabled the tests because of bug 774939

> There was an logic error on my patch on |if (!isB2G ...| so test failed on
> comment 29 (desktop builds), so re-push to try here:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=17f8462f1aef

These tests now passes with expected failure.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #30)
> Verified browser-elements tests are being run on m-3 for emulator (comment
> 27). Gonna to disable the failed tests and push again. oop tests are not
> being run; need to investigate why.

So according to

https://tbpl.mozilla.org/php/getParsedLog.php?id=32687440&tree=Try&full=1

Emulators are built with MOZ_ANDROID_OMTC=1 too.

    
    

    
  
These patches disabled failed tests in comment 27 and enable oop tests by removing |ifndef MOZ_ANDROID_OMTC|.

https://tbpl.mozilla.org/?tree=Try&rev=9d1bbf5cb350

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #32)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #30)
> > Verified browser-elements tests are being run on m-3 for emulator (comment
> > 27). Gonna to disable the failed tests and push again. oop tests are not
> > being run; need to investigate why.
> 
> So according to
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32687440&tree=Try&full=1
> 
> Emulators are built with MOZ_ANDROID_OMTC=1 too.

I was looking at the wrong log. Emulators are NOT built with MOZ_ANDROID_OMTC=1.

https://tbpl.mozilla.org/php/getParsedLog.php?id=32709504&tree=Try&full=1#error49

It simply gave up trying because there were too many timeouts in the inproc part of the tests. Gonna try to disable that and make try run a full run and disable all failed tests.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #34)
> It simply gave up trying because there were too many timeouts in the inproc
> part of the tests. Gonna try to disable that and make try run a full run and
> disable all failed tests.

https://tbpl.mozilla.org/?tree=Try&rev=0acb71eebccf

This try disables all the failed tsts in comment 27 and comment 33. Also it make TestRunner not to give up.

    
    

    
  
\O/

The oop test passes on comment 35. I am disabling the failed tests and request for review.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test for patch (obsolete)
        — 
          — Splinter Review
      

    


  
The tests will now check for transparency of the second screenshot.

It is being marked expected failure on Fx Desktop builds on all platform.

        Attachment #8345765 -
        Attachment is obsolete: true

        Attachment #8357646 -
        Flags: review?(bugs)

    
    

    
  


  
Since the test only passes on B2G Emulator, I need it to be run on TPBL.

This patch enables the entire dom/browser-elements tests to be run in B2G Emulator, but disables tests I found failing with try.

        Attachment #8357651 -
        Flags: review?(jgriffin)

    
    

    
  
Comment on attachment 8357651 [details] [diff] [review]
Patch for enabling some mozbrowser tests on B2G emulator

Review of attachment 8357651 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

        Attachment #8357651 -
        Flags: review?(jgriffin) → review+

    
    

    
  
Comment on attachment 8357646 [details] [diff] [review]
Test for patch

>+  function screenshotTaken(screenshotImageData) {
>+    screenshotImageDatas.push(screenshotImageData);
>+    if (screenshotImageDatas.length === 1) {
>       ok(true, 'Got initial non blank screenshot');
>       iframe1.src = 'data:text/html,<html>' +
>-        '<body style="background:blue">hello</body></html>';
>+        '<body style="background:transparent">hello</body></html>';
Could you move this src load to be after you have checked that 
screenshotImageData.data is what is expected

        Attachment #8357646 -
        Flags: review?(bugs) → review+

    
    

    
  


  
Patch for commit, r=smaug

        Attachment #8345200 -
        Attachment is obsolete: true

        Attachment #8359041 -
        Flags: review+

    
    

    
  


  
Part 2, tests, r=smaug with comment addressed.

        Attachment #8357646 -
        Attachment is obsolete: true

    
    

    
  


  
Part 3 enables the tests on B2G emulator, r=jgriffin

        Attachment #8357651 -
        Attachment is obsolete: true

        Attachment #8359043 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8fc3d2e02f47
https://hg.mozilla.org/mozilla-central/rev/9c97bd22aab2
https://hg.mozilla.org/mozilla-central/rev/1f13f7342ceb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Flags: needinfo?(matt.woodrow)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: