WebM demuxer does not surface alpha

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kkoorts, Assigned: kkoorts)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

2 years ago
Blocks: 944117
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Flags: needinfo?(kkoorts)
Keywords: checkin-needed

Comment 8

2 years 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

2 years 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

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

Updated

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