944117 - Implement support for WebM Alpha

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Matthew Gregan [:kinetik]]

Alpha channel support was added to WebM (and is supported in Chrome) a little while ago.  This is one feature that Flash provides for video that hasn't had a great native-HTML solution until now.

There's discussion about adding support to libnestegg here: https://github.com/kinetiknz/nestegg/issues/12, specifically there's a new branch at https://github.com/jlegg0/nestegg that has already added most (if not all) of the necessary changes.

To support this in Gecko, we need to get jlegg0's branch merged and add the required changes to content/media/WebMReader.cpp.

Comment 1

[Andy Earnshaw]

Here's a demo you can use to compare Chrome vs Firefox:

http://simpl.info/videoalpha/

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Hallvord - are you aware of web sites using or wanting to use this feature?

Comment 3

[Hallvord R. M. Steen [:hallvors]]

I've never come across sites depending on this. It seems to be a Blink-only feature at this point, videos run without alpha in Firefox and don't run at all in Safari and IE.

Comment 4

[Andy Earnshaw]

I work for an ad serving company and we get requests through from clients every now and then.  The feature would fill the gap for people who had transparent Flash videos.

Comment 5

[Michael]

We are using this feature in Chrome to display animated cards as a video over a textured background. It would be great to have support in Firefox, as we currently use GIFs to achieve the same effect. 

For reference: 
http://i.imgur.com/wjaSZe2.jpg (Animated cards are a video over the textured background) 
http://www.hearthpwn.com/packs/5099-easy

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Obsoleting Flash seems like a compelling reason to do this.

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The first step is to land and uplift some telemetry so we can measure how often this feature is being used.

Comment 8

[Matthew Gregan [:kinetik]]

Chrome just added support for VP9 alpha, which uses the codec's support for alpha rather than the side-by-side WebM technique used for VP8.  Changeset here: https://codereview.chromium.org/2096813002/

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #8)
> Chrome just added support for VP9 alpha, which uses the codec's support for
> alpha rather than the side-by-side WebM technique used for VP8.  Changeset
> here: https://codereview.chromium.org/2096813002/

Actually, bear-vp9a.webm has BlockAdditions, so it's using the side-by-side WebM technique like VP8.  The libvpx codec support for alpha was removed before it became stable.

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Michael from comment #5)
> We are using this feature in Chrome to display animated cards as a video
> over a textured background. It would be great to have support in Firefox, as
> we currently use GIFs to achieve the same effect. 
> 
> For reference: 
> http://i.imgur.com/wjaSZe2.jpg (Animated cards are a video over the textured
> background) 
> http://www.hearthpwn.com/packs/5099-easy

It appears that we're now getting png files instead of gifs here. So we have no animation, though I have seen pop in of the webms briefly before them being replaced with static images.

Comment 12

[Michael]

(In reply to Bryce Van Dyk (:SingingTree) from comment #11)

> It appears that we're now getting png files instead of gifs here. So we have
> no animation, though I have seen pop in of the webms briefly before them
> being replaced with static images.

Yeah, it looks like at some point this year we moved away from gifs on that page type. You can still see the gif vs webm in action on http://www.hearthpwn.com/cards/42049-arcane-giant

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Cheers for that. This is something we're looking to have picked up soon, so I'm going through and making sure information is up to date so this is a smooth pick up for the person working on it. Good to have a reference to work with, and appreciate your response!

Comment 14

[Karolien]

Attachment: Bug 944117 - updated WebM demuxer to surface alpha information.

Review commit: https://reviewboard.mozilla.org/r/94810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94810/

Comment 15

[Karolien]

Comment on attachment 8813407 [details]
Bug 944117 - updated WebM demuxer to surface alpha information.

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

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8813407 [details]
Bug 944117 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/94810/#review95046

Very impressed at the quality of your first submission !

The change of the method Size name, makes it that over 80% of the patch size is actually unrelated to the work at hand.

I'd strongly prefer for the name to not be changed.
If you do want to change it, please do so in a separate commit.

So that 1 commit == 1 scope *exactly*, as per the media review guidelines.

::: dom/media/MediaData.h:621
(Diff revision 2)
>  
>  class MediaRawData : public MediaData {
>  public:
>    MediaRawData();
>    MediaRawData(const uint8_t* aData, size_t mSize);
> +  MediaRawData(const uint8_t* aData, size_t mSize, const uint8_t* aAlphaData, size_t mAlphaSize);

Please have a read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

80 columns width

::: dom/media/MediaData.h:628
(Diff revision 2)
>    // Pointer to data or null if not-yet allocated
>    const uint8_t* Data() const { return mBuffer.Data(); }
> +  // Pointer to alpha data or null if not-yet allocated
> +  const uint8_t* AlphaData() const { return mAlphaBuffer.Data(); }
>    // Size of buffer.
> -  size_t Size() const { return mBuffer.Length(); }
> +  size_t DataSize() const { return mBuffer.Length(); }

Please keep the original Size method name, especially as you've kept the just Data() method

it makes your changes way too big otherwise and totally out of scope.

The minimum change at a time the better.

https://docs.google.com/document/d/1S-CKP4LJyGhxZBQ1gxOhpMo-rZWW29IK3rYONakKsnY/edit

::: dom/media/MediaInfo.h:310
(Diff revision 2)
>  
>    // Describing how many degrees video frames should be rotated in clock-wise to
>    // get correct view.
>    Rotation mRotation;
>  
> +  // Indicates whether or not frames may contain alpha information.

We're trying to remove accessing the members directly and instead prefer access methods.

So recently all changes made to TrackInfo were done using accessor.

Please make mAlphaPresent a private member, and make an accessor HasAlpha()

::: dom/media/webm/WebMDemuxer.cpp:633
(Diff revision 2)
>        return false;
>      }
> +    unsigned char* alphaData;
> +    size_t alphaLength = 0;
> +    // Check packets for alpha information if file has declared alpha frames may be present.
> +    if (mInfo.mVideo.mAlphaPresent == true) {

don't check explicit value with boolean

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
"Do not compare x == true or x == false. Use (x) or (!x) instead. x == true, in fact, is different from if (x)!"

::: dom/media/webm/WebMDemuxer.cpp:674
(Diff revision 2)
>      }
>  
>      WEBM_DEBUG("push sample tstamp: %ld next_tstamp: %ld length: %ld kf: %d",
>                 tstamp, next_tstamp, length, isKeyframe);
> -    RefPtr<MediaRawData> sample = new MediaRawData(data, length);
> +    RefPtr<MediaRawData> sample;
> +    if (mInfo.mVideo.mAlphaPresent == true && alphaLength != 0) {

no need to test == true, being a bool just:
if (mInfo.mVideo.mAlphaPresent)

which now would be if (mInfo.mVideo.HasAlpha() ...

Comment 17

[Karolien]

Attachment: Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

Review commit: https://reviewboard.mozilla.org/r/94868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94868/

Comment 18

[Karolien]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

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

Comment 19

[Karolien]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94868/diff/2-3/

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

https://reviewboard.mozilla.org/r/94868/#review95090

::: dom/media/MediaData.h:635
(Diff revision 3)
>    // Size of buffer.
>    size_t Size() const { return mBuffer.Length(); }
> +  size_t AlphaSize() const { return mAlphaBuffer.Length(); }
>    size_t ComputedSizeOfIncludingThis() const
>    {
> -    return sizeof(*this) + mBuffer.ComputedSizeOfExcludingThis();
> +    return sizeof(*this) +

return sizeof(*this) +
       mBuffer.ComputedSizeOfExcludingThis() +
       mAlphaBuffer.ComputedSizeOfExcludingThis();

should be:

return sizeof(*this)
       + mBuffer.ComputedSizeOfExcludingThis()
       + mAlphaBuffer.ComputedSizeOfExcludingThis();

from coding style:
"Operators

Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?:, binary arithmetic operators including +, and member-of operators (in particular the . operator in JavaScript, see the Rationale)."

::: dom/media/MediaData.cpp:396
(Diff revision 3)
>    , mCrypto(mCryptoInternal)
>    , mBuffer(aData, aSize)
>  {
>  }
>  
> +MediaRawData::MediaRawData(const uint8_t* aData,

personally, I would try to group those that is more like:

MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
                           const uint8_t* aAlphaData, size_t aAlphaSize)

as aSize and aData are linked to one another.

but that's fine if you don't.

::: dom/media/MediaInfo.h:321
(Diff revision 3)
>    // Describing how many degrees video frames should be rotated in clock-wise to
>    // get correct view.
>    Rotation mRotation;
>  
>  private:
> +  // Indicates whether or not frames may contain alpha information.

please add the new members below existing ones.

(make sure the order match in the constructor initialization)

::: dom/media/webm/WebMDemuxer.cpp:361
(Diff revision 3)
>        }
>  
>        mVideoTrack = track;
>        mHasVideo = true;
>  
> +      mInfo.mVideo.SetAlpha(params.alpha_mode);

you're adding to existing code, I would prefer if logically that line was below mInfo.mVideo.SetImageRect(pictureRect)

::: dom/media/webm/WebMDemuxer.cpp:635
(Diff revision 3)
> +    unsigned char* alphaData;
> +    size_t alphaLength = 0;
> +    // Check packets for alpha information if file has declared alpha frames
> +    // may be present.
> +    if (mInfo.mVideo.HasAlpha()) {
> +      nestegg_packet_additional_data(holder->Packet(),

per the doc:

nestegg_packet_additional_data returns an error code.

Please check that the value isn't -1 (like all other use of nestegg commands)

Comment 21

[Karolien]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

https://reviewboard.mozilla.org/r/94868/#review95090

> personally, I would try to group those that is more like:
> 
> MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
>                            const uint8_t* aAlphaData, size_t aAlphaSize)
> 
> as aSize and aData are linked to one another.
> 
> but that's fine if you don't.

Do you mean like this? 

data, size
alphaData, alphaSize

As when it's in one line it goes over the 80 limit.

Comment 22

[Karolien]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94868/diff/3-4/

Comment 23

[Karolien]

Comment on attachment 8813463 [details]
Bug 944117 - updated WebM demuxer to surface alpha information. f?jya

Moved to bug 1320829. Going to use this bug for tracking overall feature and will raise new bugs as necessary.

Comment 24

[Karolien]

Hi Michael,

Could you please confirm that WebM alpha is working correctly in Nightly on Hearthpwn? It looks like it's working on my end, I just need to spoof my user agent to get WebM served.

Comment 25

[Michael]

(In reply to Karolien from comment #24)
> Hi Michael,
> 
> Could you please confirm that WebM alpha is working correctly in Nightly on
> Hearthpwn? It looks like it's working on my end, I just need to spoof my
> user agent to get WebM served.

Everything looks good on Hearthpwn with Nightly and a spoofed user agent. Thanks!

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Release Note Request (optional, but appreciated)
[Why is this notable]: Video with alpha channel seems to be something developers may be interested in.
[Affects Firefox for Android]: Not sure. This bug doesn't seem to be desktop-specific unless we disable WebM on Android.
[Suggested wording]: Add support for WebM video with alpha
[Links (documentation, blog post, etc)]: Document in http://wiki.webmproject.org/alpha-channel, and Chrome has a blog post about this feature several years ago: https://developers.google.com/web/updates/2013/07/Alpha-transparency-in-Chrome-video

Comment 27

[Karolien]

Release Note Request:

[Why is this notable]: WebM videos with alpha is of interest to developers. Websites such as Hearthpwn are already using it, see http://www.hearthpwn.com/packs/5099-easy (currently images are static for Firefox user agent.) WebM videos with alpha serve the same purpose as GIFs, but are of better quality and compression.
[Affects Firefox for Android]: Yes. WebM videos with alpha channel are served on Firefox for Android.
[Suggested wording]: Add support for WebM video with alpha
[Links (documentation, blog post, etc)]: Document in http://wiki.webmproject.org/alpha-channel, and Chrome has a blog post about this feature several years ago: https://developers.google.com/web/updates/2013/07/Alpha-transparency-in-Chrome-video

Comment 28

[Jim Black]

This works for me on Windows (10), but on Linux (Debian Stretch, 64-bit) I'm seeing some issues, which appear to be specific to these transparent videos.

For videos opened in their own tab:
https://simpl.info/videoalpha/video/dancer1.webm
http://media-hearth.cursecdn.com/goldCards/0/303/303.webm
1. Pausing the video causes it to become transparent.
2. A looping video has an annoying transparent flash each time it passes the end of the video.

For videos embedded in a web page:
https://simpl.info/videoalpha/
1. Pausing the video followed by clicking elsewhere to remove focus from the video causes it to become transparent.
2. Looping videos flash, same as in the own-tab case.

Tested in
53.0a1 (2017-01-14) (64-bit)
both in a fresh profile, and in an otherwise fresh profile with hardware acceleration disabled.

Comment 29

[Florian Scholz (Open Web Docs)]

Thanks for setting dev-doc-needed, Ralph. In which release does this ship? Can you set the target milestone field, please? Thank you!

Comment 30

[Ralph Giles (:rillian)]

This feature ships in Firefox 53. Sorry for the confusion!

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Noted for 53, as "Support for WebM video with alpha, which allows playing videos with transparent backgrounds".  Ralph, does this also ship for Firefox for Android? Is WebM support in fennec? Thanks.

Comment 32

[Ralph Giles (:rillian)]

Fennec supports WebM. The transparency feature is supposed to work there, but there's a positioning bug on my phone.

Comment 33

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #32)
> Fennec supports WebM. The transparency feature is supposed to work there,
> but there's a positioning bug on my phone.

Nevermind, the positioning bug is an issue with the test layout I was using to verify. This is available on desktop and android for Firefox 53.

Comment 34

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Implement_support_for_WebM_Alpha