1144103 - Support screen recording

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[:gerard-majax]

Hacking around in KK, I've sound the |screenrecord| binary on my device. This is built from https://android.googlesource.com/platform/frameworks/av/+/kitkat-mr2.2-release/cmds/screenrecord/screenrecord.cpp

Running it quickly returns me an error:
> ERROR: unable to get display characteristics

Meaning the call to |err = SurfaceComposerClient::getDisplayInfo(mainDpy, &mainDpyInfo);| failed.

I don't know well this part of the system, can we easily leverage this to support screen recording ?

Comment 1

[Sotaro Ikeda [:sotaro]]

ISurfaceComposer's implementation is FakeSurfaceComposer. It implement only necessary few api now in gecko. We need to connect it to GonkDisplayJB and LayerManagerComposite need to support multiple rendering target.

gerard-majax, can I take the bug? This might becomes necessary as basic capability for another capability like WifiDisplay.

Comment 2

[Sotaro Ikeda [:sotaro]]

Android's related part is like the following.
https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/DisplayDevice_4.4.pdf?raw=true

Comment 3

[:gerard-majax]

Feel tree to work on this :)

Comment 4

[No-Jun Park [:njpark]]

This feature would be really great for automated QA testing, and I hope it gets included in 3.0 feature lists as well.  I believe this is essentially same as the eideticker setup without the need for external camera, and perhaps we can do advanced image comparison tests with it.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Implementing by using nexus-5-l. Recording works, but when finish recording, b2g process reboot.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix crash when end recording. Removed redundant logs.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

correct patch.

Comment 8

[Sotaro Ikeda [:sotaro]]

attachment 8589163 [details] [diff] [review] have many changes. It seems better to handle some problems by sub bugs.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

un-bitrot.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

un-bitrot.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Update nits.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 14

[Bobby Chien]

Sotaro, do we have workable screenrecord tool for Aries? Thanks.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add Screen recording capability

Rebase and kk support.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch for kk - Change to framework/av screenreord.cpp

Picked change from L. Without this patch. screen record automatically finished very quickly.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Bobby Chien [:bchien] from comment #14)
> Sotaro, do we have workable screenrecord tool for Aries? Thanks.

By applying attachment 8682947 [details] [diff] [review] and attachment 8682950 [details] [diff] [review]. I confirmed screenrecord on master aries(KK).

Comment 18

[Bobby Chien]

Sotaro, you mean your patch in comment 17 already landed on master/firefox45, right? Thanks.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Bobby Chien [:bchien] from comment #18)
> Sotaro, you mean your patch in comment 17 already landed on
> master/firefox45, right? Thanks.

They are not landed to yet.

Comment 20

[Sotaro Ikeda [:sotaro]]

I created the patches in comment 17 yesterday and applied local master aries environment.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Rebased.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Code clean up.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Add a comment.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Fix build failure on JB.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Add pref check.

Comment 26

[Michael Wu [:mwu]]

Comment on attachment 8687789 [details] [diff] [review]
patch - Add Screen recording capability

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

Looks fine overall, though I guess we might want to rename FakeSurfaceComposer to GeckoSurfaceComposer later since so much is implemented now. r=me with nits, locking, and density detection fixed.

::: widget/gonk/nativewindow/FakeSurfaceComposer.cpp
@@ +78,5 @@
> +        MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +        RefPtr<nsScreenManagerGonk> screenManager =
> +            nsScreenManagerGonk::GetInstance();
> +        screenManager->RemoveScreen(GonkDisplay::DISPLAY_VIRTUAL);
> +        mComposer->mDisplays.removeItemsAt(mIndex);

Displays are added to mDisplay on another thread but removed on the main thread. A lock is taken for adding the display, but not for removal, so I'm not sure how it's safe.

@@ +107,5 @@
> +            : composer(composer) {
> +        }
> +    };
> +
> +    if(!gfxPrefs::ScreenRecordingEnabled()) {

nit: space after if

@@ +178,5 @@
> +
> +    uint32_t flags = 0;
> +    DisplayDeviceState& disp(mDisplays.editValueAt(dpyIdx));
> +
> +    if (disp.isValid()) {

nit: early return here to avoid an indenting the rest of the function.

@@ +411,5 @@
> +    if (Density::getEmuDensity()) {
> +        // if "qemu.sf.lcd_density" is specified, it overrides everything
> +        xdpi = ydpi = density = Density::getEmuDensity();
> +        density /= 160.0f;
> +    }

I think we should remove all this density logic and just use screen->GetDpi(). That's what gecko actually thinks the DPI is.

::: widget/gonk/nativewindow/FakeSurfaceComposer.h
@@ +31,5 @@
> +#include <hardware/hwcomposer.h>
> +#include <private/gui/LayerState.h>
> +#include <utils/KeyedVector.h>
> +
> +#include "nsCOMPtr.h"

Does this need to be here? I don't see nsCOMPtrs used in this header.

@@ +128,5 @@
> +
> +    struct DisplayDeviceState {
> +        enum {
> +            NO_LAYER_STACK = 0xFFFFFFFF,
> +         };

nit: indentation

@@ +152,5 @@
> +    };
> +
> +    // access must be protected by mStateLock
> +    mutable Mutex mStateLock;
> +    DefaultKeyedVector< wp<IBinder>, DisplayDeviceState> mDisplays;

nit: remove the space before wp, if possible.

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #26)
> Comment on attachment 8687789 [details] [diff] [review]
> patch - Add Screen recording capability
> 
> Review of attachment 8687789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine overall, though I guess we might want to rename
> FakeSurfaceComposer to GeckoSurfaceComposer later since so much is
> implemented now. r=me with nits, locking, and density detection fixed.

I am going to update the class name in a different bug.

> 
> ::: widget/gonk/nativewindow/FakeSurfaceComposer.cpp
> @@ +78,5 @@
> > +        MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> > +        RefPtr<nsScreenManagerGonk> screenManager =
> > +            nsScreenManagerGonk::GetInstance();
> > +        screenManager->RemoveScreen(GonkDisplay::DISPLAY_VIRTUAL);
> > +        mComposer->mDisplays.removeItemsAt(mIndex);
> 
> Displays are added to mDisplay on another thread but removed on the main
> thread. A lock is taken for adding the display, but not for removal, so I'm
> not sure how it's safe.

Thanks, loch is necessary for removal. I will update it in a next patch.

> 
> @@ +107,5 @@
> > +            : composer(composer) {
> > +        }
> > +    };
> > +
> > +    if(!gfxPrefs::ScreenRecordingEnabled()) {
> 
> nit: space after if

I'll update it in a next patch.

> 
> @@ +178,5 @@
> > +
> > +    uint32_t flags = 0;
> > +    DisplayDeviceState& disp(mDisplays.editValueAt(dpyIdx));
> > +
> > +    if (disp.isValid()) {
> 
> nit: early return here to avoid an indenting the rest of the function.

I'll update it in a next patch.

> 
> @@ +411,5 @@
> > +    if (Density::getEmuDensity()) {
> > +        // if "qemu.sf.lcd_density" is specified, it overrides everything
> > +        xdpi = ydpi = density = Density::getEmuDensity();
> > +        density /= 160.0f;
> > +    }
> 
> I think we should remove all this density logic and just use
> screen->GetDpi(). That's what gecko actually thinks the DPI is.

Thanks. I'll update in a next patch.

> 
> ::: widget/gonk/nativewindow/FakeSurfaceComposer.h
> @@ +31,5 @@
> > +#include <hardware/hwcomposer.h>
> > +#include <private/gui/LayerState.h>
> > +#include <utils/KeyedVector.h>
> > +
> > +#include "nsCOMPtr.h"
> 
> Does this need to be here? I don't see nsCOMPtrs used in this header.

Until previous patch, it was necessary. I'll update it in a next patch.

> 
> @@ +128,5 @@
> > +
> > +    struct DisplayDeviceState {
> > +        enum {
> > +            NO_LAYER_STACK = 0xFFFFFFFF,
> > +         };
> 
> nit: indentation

I'll update it in a next patch.

> 
> @@ +152,5 @@
> > +    };
> > +
> > +    // access must be protected by mStateLock
> > +    mutable Mutex mStateLock;
> > +    DefaultKeyedVector< wp<IBinder>, DisplayDeviceState> mDisplays;
> 
> nit: remove the space before wp, if possible.

I'll update it in a next patch.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Screen recording capability

Apply the comment. Carry "r=mwu".

Comment 29

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86dadb2dd2ba

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b46bd094971d

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b46bd094971d

Comment 32

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Hey Sotaro, could you quickly share how this works ? A pointer to an Internet resource works too :)

Comment 33

[:gerard-majax]

Flip the pref layers.screen-recording.enabled, then adb shell; screenrecord /path/to/mp4

Comment 35

[Sotaro Ikeda [:sotaro]]

(In reply to Alexandre LISSY :gerard-majax from comment #33)
> Flip the pref layers.screen-recording.enabled, then adb shell; screenrecord /path/to/mp4

Thanks. It should works on gonk kk and gonk L devices.

Comment 36

[Sotaro Ikeda [:sotaro]]

I checked only on nexus-5-l and aries. I am going to check it on flame-kk.

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> I checked only on nexus-5-l and aries. I am going to check it on flame-kk.

flame-kk also works by "adb shell screenrecord /sdcard/screenrecord.mp4" with the pref enabled.

Comment 38

[Bobby Chien]

Hi Sotaro, I can't find layers.screen-recording.enabled in latest nexus-5-l build. Any trick? :)

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Bobby Chien [:bchien] from comment #38)
> Hi Sotaro, I can't find layers.screen-recording.enabled in latest nexus-5-l
> build. Any trick? :)

It is because it is disabled by default.
 https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#317

Comment 40

[Bobby Chien]

Sotaro, nexus 5 screenrecord is 16 fps per my own testing. Does it able to record as 30 or 60 fps? Thanks.

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to Bobby Chien [:bchien] from comment #40)
> Sotaro, nexus 5 screenrecord is 16 fps per my own testing. Does it able to
> record as 30 or 60 fps? Thanks.

Without modification of the code, it might be not possible. Optimization rendering path need to be implemented.

Comment 42

[No-Jun Park [:njpark]]

Thanks so much for this sotaro!