Implement support for WebM Alpha

RESOLVED FIXED in mozilla53

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: kinetik, Assigned: Karolien)

Tracking

({compat, dev-doc-needed})

Trunk
mozilla53
compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 53+)

Details

(URL)

Attachments

(2 obsolete attachments)

(Reporter)

Description

4 years ago
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

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

http://simpl.info/videoalpha/
Component: Audio/Video → Audio/Video: Playback
Hallvord - are you aware of web sites using or wanting to use this feature?
Flags: needinfo?(hsteen)
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.
Flags: needinfo?(hsteen)

Comment 4

a year ago
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

a year ago
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
Obsoleting Flash seems like a compelling reason to do this.
Priority: -- → P2
The first step is to land and uplift some telemetry so we can measure how often this feature is being used.
Keywords: compat
(Reporter)

Comment 8

a year ago
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/
Mass change P2 -> P3
Priority: P2 → P3
(Reporter)

Comment 10

9 months ago
(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.
(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

8 months ago
(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
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

7 months ago
mozreview-review
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() ...
Attachment #8813407 - Flags: review?(jyavenard) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8813407 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

7 months ago
mozreview-review
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)
Attachment #8813463 - Flags: review+
(Assignee)

Comment 21

7 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Assignee: nobody → kkoorts
(Assignee)

Updated

7 months ago
Depends on: 1320829
(Assignee)

Comment 23

7 months ago
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.
Attachment #8813463 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Depends on: 1321076
(Assignee)

Updated

6 months ago
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
(Assignee)

Comment 24

6 months ago
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.
Flags: needinfo?(chaudharymic)

Comment 25

6 months ago
(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!
Flags: needinfo?(chaudharymic)
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
relnote-firefox: --- → ?
(Assignee)

Comment 27

5 months ago
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

5 months ago
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.
Keywords: dev-doc-needed
(Assignee)

Updated

5 months ago
Depends on: 1332952
Thanks for setting dev-doc-needed, Ralph. In which release does this ship? Can you set the target milestone field, please? Thank you!
Flags: needinfo?(giles)
Depends on: 1339792
Depends on: 1339830
This feature ships in Firefox 53. Sorry for the confusion!
Flags: needinfo?(giles)
Target Milestone: --- → mozilla53
(Assignee)

Updated

4 months ago
No longer depends on: 1339792
(Assignee)

Updated

4 months ago
No longer depends on: 1339830
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.
relnote-firefox: ? → 53+
Flags: needinfo?(giles)
Fennec supports WebM. The transparency feature is supposed to work there, but there's a positioning bug on my phone.
Flags: needinfo?(giles)
(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.
[Test Plan]:
https://wiki.mozilla.org/QA/Implement_support_for_WebM_Alpha
QA Contact: mihai.boldan
You need to log in before you can comment on or make changes to this bug.