WebM demuxer does not surface alpha

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: Karolien, Assigned: Karolien)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
To implement alpha in WebM-contained videos, the demuxer must first surface the alpha information.
(Assignee)

Updated

9 months ago
Blocks: 944117
Comment hidden (mozreview-request)
(Assignee)

Comment 2

9 months ago
Original review granted on bug 944117 https://reviewboard.mozilla.org/r/94868/diff/4#index_header
Created this bug in order to leave other open as tracking bug for overall feature.
Keywords: checkin-needed

Comment 3

9 months ago
mozreview-review
Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

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

MediaRawData(const uint8_t* aData, size_t mSize,
               const uint8_t* aAlphaData, size_t aAlphaSize)
               
also fit in 80 columns

::: dom/media/MediaData.h:624
(Diff revision 1)
>    MediaRawData();
>    MediaRawData(const uint8_t* aData, size_t mSize);
> +  MediaRawData(const uint8_t* aData,
> +               size_t mSize,
> +               const uint8_t* aAlphaData,
> +               size_t mAlphaSize);

aAlphaSize

it was incorrectly set to mSize to, please fix that while you're at it.

::: dom/media/MediaData.cpp:399
(Diff revision 1)
>  }
>  
> +MediaRawData::MediaRawData(const uint8_t* aData,
> +                           size_t aSize,
> +                           const uint8_t* aAlphaData,
> +                           size_t aAlphaSize)

MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
                           const uint8_t* aAlphaData, size_t aAlphaSize)
  : MediaData(RAW_DATA, 0)
  , mCrypto(mCryptoInternal)
  , mBuffer(aData, aSize)
  , mAlphaBuffer(aAlphaData, aAlphaSize)
{

this fit in 80 columns here (73)

::: dom/media/MediaInfo.h:326
(Diff revision 1)
>    // mImage may be cropped; currently only used with the WebM container.
>    // A negative width or height indicate that no cropping is to occur.
>    nsIntRect mImageRect;
> +
> +  // Indicates whether or not frames may contain alpha information.
> +  bool mAlphaPresent;

you could favour C++11 inline construction:

bool mAlphaPresent = false;

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

argument alignment is wrong

::: dom/media/webm/WebMDemuxer.cpp:642
(Diff revision 1)
> +                                     &alphaData,
> +                                     &alphaLength);
> +      if (r == -1) {
> +        WEBM_DEBUG(
> +          "nestegg_packet_additional_data failed to retrieve alpha data r=%d",
> +          r);

shouldn't this be a fatal error?

or is it possible that not all frames contain alpha plane?

if not, please return error and make this a fatal error.
Attachment #8815114 - Flags: review?(jyavenard) → review+
Please address the comments first.
Keywords: checkin-needed
Flags: needinfo?(kkoorts)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

9 months ago
mozreview-review
Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96096/#review96284
(Assignee)

Comment 7

9 months ago
mozreview-review-reply
Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

> MediaRawData(const uint8_t* aData, size_t mSize,
>                const uint8_t* aAlphaData, size_t aAlphaSize)
>                
> also fit in 80 columns

Hi sorry it won't fit given the format of the file so I've split the arguments over multiple lines.

> MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
>                            const uint8_t* aAlphaData, size_t aAlphaSize)
>   : MediaData(RAW_DATA, 0)
>   , mCrypto(mCryptoInternal)
>   , mBuffer(aData, aSize)
>   , mAlphaBuffer(aAlphaData, aAlphaSize)
> {
> 
> this fit in 80 columns here (73)

Hi are you referring to the signature? I think it is 103 characters so it won't fit without being split.

> shouldn't this be a fatal error?
> 
> or is it possible that not all frames contain alpha plane?
> 
> if not, please return error and make this a fatal error.

As per the spec, it is possible that the header declares alpha is present and that there are frames that do not contain alpha, so it should not be a fatal error.
(Assignee)

Updated

9 months ago
Flags: needinfo?(kkoorts)
Keywords: checkin-needed

Comment 8

9 months ago
mozreview-review-reply
Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

> Hi are you referring to the signature? I think it is 103 characters so it won't fit without being split.

Seems that reviewboard didn't include my CR..

so let's do it again.

I modified the file locally, and it's definitely not over 80 lines.

MediaRawData::MediaRawData(const uint8_t aData, size_t aSize,
                           const uint8_t aAlphaData, size_t aAlphaSize)
  : MediaData(RAW_DATA, 0)
  , mCrypto(mCryptoInternal)
  , mBuffer(aData, aSize)
  , mAlphaBuffer(aAlphaData, aAlphaSize)
{


same in the header.
Comment hidden (mozreview-request)

Comment 10

9 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c599df1589cc
updated WebM demuxer to surface alpha information. r=jya
Keywords: checkin-needed

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c599df1589cc
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

8 months ago
Depends on: 1323847
Assignee: nobody → kkoorts
You need to log in before you can comment on or make changes to this bug.