Bug 1314147 (AV1)

Support the AOMedia Video 1 (AV1) codec format

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
9 months ago
a month ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

(Blocks: 2 bugs, {feature})

unspecified
mozilla55
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(8 attachments, 5 obsolete attachments)

59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
kinetik
: review+
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
kinetik
: review+
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
kinetik
: review+
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
(Assignee)

Description

9 months ago
Tracking bug for supporting the Alliance for Open Media's av1 codec.

https://aomedia.googlesource.com/aom/
http://aomedia.org/
(Assignee)

Updated

9 months ago
Assignee: nobody → giles
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
See Also: → bug 1332136
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

3 months ago
FYI: --enable-av1 builds fine on FreeBSD 12 (Clang 4.0 or GCC 6.3).
(Assignee)

Comment 19

3 months ago
That's great to hear. Thanks for testing so quickly!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

3 months ago
mozreview-review
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review134722

::: media/libaom/moz.build:12
(Diff revision 4)
> +with Files('*'):
> +    BUG_COMPONENT = ('Core', 'Audio/Video')
> +
> +include('sources.mozbuild')
> +
> +if CONFIG['VPX_USE_YASM']:

`VPX_USE_YASM` isn't defined for `--with-system-libvpx`. Maybe replace with `HAVE_YASM` + `INTEL_ARCHITECTURE` or adjust old-configure.in while keeping in mind `--with-system-aom` maybe added in future.

```
/usr/bin/clang -std=gnu99 -o aom_convolve_copy_sse2.o -fPIC -Wa,--noexecstack -include objdir/mozilla-config.h -DMOZILLA_CLIENT  -Imedia/libaom/config/linux/x64/ -I. -Ithird_party/aom  -c third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm
clang: warning: argument unused during compilation: '-std=gnu99' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-include objdir/mozilla-config.h' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-D MOZILLA_CLIENT' [-Wunused-command-line-argument]
third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm:2:17: error: unexpected token in argument list
; Copyright (c) 2016, Alliance for Open Media. All rights reserved
                ^
third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm:4:15: error: unexpected token in argument list
; This source code is subject to the terms of the BSD 2 Clause License and
              ^
[...]
```
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

3 months ago
With this patch set, I can play back https://people-mozilla.org/~rgiles/2017/rush_hour_444.av1.webm although the aspect ratio is wrong.

Another file crashes with a simd alignment assert: https://people-mozilla.org/~rgiles/2017/sintel_trailer_2k_480p24.av1.webm

Still, a happy milestone.
(Assignee)

Updated

3 months ago
Depends on: 1358662
(Assignee)

Updated

3 months ago
Depends on: 1360396
Alias: AV1
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Keywords: feature
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Support the av1 video codec → Support the AOMedia Video 1 (AV1) codec format
(Assignee)

Comment 43

3 months ago
(In reply to Jan Beich from comment #28)

> `VPX_USE_YASM` isn't defined for `--with-system-libvpx`. Maybe replace with
> `HAVE_YASM` + `INTEL_ARCHITECTURE` or adjust old-configure.in

Thanks for testing. I'm happy to do this. Unfortunately VPX_USE_ASM is the easy part...

VPX_AS_CONVERSION is fairly simple to redefine locally in the moz.build: it's defined on arm when GNU_AS is defined. Are you aware of any situations where GNU_AS wouldn't be available?

The hard part is VPX_AS_FLAGS, which has a several bits to it, and also isn't defined with --with-system-libvpx. I'm not sure what to do about this case besides moving all that logic into the moz.configure side and making it available separately to both the libvpx and libaom moz.build files. That may have to wait until after this lands.
(Assignee)

Comment 44

3 months ago
Actually, it looks like the USE_YASM setting here is dead code. I don't see a subsequent use. The ASFLAGS are still a problem though.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8860177 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8860179 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8860181 - Attachment is obsolete: true

Comment 55

3 months ago
mozreview-review
Comment on attachment 8859732 [details]
Bug 1314147 - Add AOMDecoder.

https://reviewboard.mozilla.org/r/131728/#review139144

::: dom/media/platforms/agnostic/AOMDecoder.cpp:29
(Diff revision 3)
> +using namespace gfx;
> +using namespace layers;
> +
> +
> +static nsresult
> +InitContext(aom_codec_ctx_t* aCtx,

please use MediaResult so additional details can be returned and reported back to the user.

::: dom/media/platforms/agnostic/AOMDecoder.cpp:46
(Diff revision 3)
> +  else if (aInfo.mDisplay.width >= 1024) {
> +    decode_threads = 4;
> +  }
> +  decode_threads = std::min(decode_threads, PR_GetNumberOfProcessors());
> +
> +  aom_codec_dec_cfg_t config;

maybe using config() or PodZero to ensure the structure is fully initialised?

::: dom/media/platforms/agnostic/AOMDecoder.cpp:55
(Diff revision 3)
> +  aom_codec_flags_t flags = 0;
> +
> +  auto res = aom_codec_dec_init(aCtx, dx, &config, flags);
> +  if (res != AOM_CODEC_OK) {
> +    LOG_RESULT(res, "Codec initialization failed!");
> +    return NS_ERROR_FAILURE;

with MediaResult additional information can be returned to fully describe where it failed and why... such as returning the AOM error code.

::: dom/media/platforms/agnostic/AOMDecoder.cpp:65
(Diff revision 3)
> +AOMDecoder::AOMDecoder(const CreateDecoderParams& aParams)
> +  : mImageContainer(aParams.mImageContainer)
> +  , mTaskQueue(aParams.mTaskQueue)
> +  , mInfo(aParams.VideoConfig())
> +{
> +  MOZ_COUNT_CTOR(AOMDecoder);

This is rather unnecessary as the MediaDataDecoder is already ref counted and those are automally moz_count

::: dom/media/webm/WebMDemuxer.cpp:30
(Diff revision 3)
>  #include "mozilla/IntegerPrintfMacros.h"
>  #include "mozilla/SizePrintfMacros.h"
>  #include "mozilla/Sprintf.h"
>  
>  #include <algorithm>
> +#include <stdint.h>

this seems out of scope for this patch....
Attachment #8859732 - Flags: review?(jyavenard) → review+

Comment 56

3 months ago
mozreview-review
Comment on attachment 8859733 [details]
Bug 1314147 - Add AOMDecoder to AgnosticDecoderModule.

https://reviewboard.mozilla.org/r/131730/#review139148
Attachment #8859733 - Flags: review?(jyavenard) → review+

Comment 57

3 months ago
mozreview-review
Comment on attachment 8860178 [details]
Bug 1314147 - Recognize AV1 in WebMDemuxer.

https://reviewboard.mozilla.org/r/132202/#review139142

::: dom/media/webm/WebMDemuxer.cpp:336
(Diff revision 2)
>            break;
>          case NESTEGG_CODEC_VP9:
>            mInfo.mVideo.mMimeType = "video/webm; codecs=vp9";
>            break;
> +        case NESTEGG_CODEC_AV1:
> +          mInfo.mVideo.mMimeType = "video/webm; codecs=av1";

I really don't like those mimetype...

both vp8, vp9 have an officiel mime type:
video/x-vnd.on2.vp8 and video/x-vnd.on2.vp9

does av1 have its own?
Attachment #8860178 - Flags: review?(jyavenard) → review+

Comment 58

3 months ago
mozreview-review
Comment on attachment 8847833 [details]
Bug 1314147 - Add 'mach vendor aom' for maintaining av1 codec support.

https://reviewboard.mozilla.org/r/120750/#review139212

This looks fine, some things to discuss before this is landed below.

::: python/mozbuild/mozbuild/mach_commands.py:1841
(Diff revision 7)
> +    @CommandArgument('-r', '--revision',
> +        help='Repository tag or commit to update to.')

The update scripts I'm familiar with around the tree usually document the version that was last downloaded in the source; I think this practice is a little more robust than recording the version used in a commit message.

I suppose having to change the source code means having to always pass `--ignore-modified` to this command (or change the defaults, e.g. ignoring the vendoring module always), which may not be desirable.

::: python/mozbuild/mozbuild/vendor_aom.py:34
(Diff revision 7)
> +        tar = tarfile.open(filename)
> +        self.log(logging.INFO, 'rm_vendor_dir', {}, 'rm -rf %s' % target)
> +        mozfile.remove(target)
> +        self.log(logging.INFO, 'unpack', {}, 'Unpacking upstream files.')
> +        tar.extractall(target)

Should we add some checking here to make sure the archive unpacks in roughly the way that we expect?  I guess given the source (the tarballs are generated from the git repo itself?), maybe we don't have to worry about that...

::: python/mozbuild/mozbuild/vendor_aom.py:70
(Diff revision 7)
> +        if not revision:
> +            revision = 'master'

Re: versioning, I don't think we want to default to downloading `master`, because that makes it too easy to lose track of what version we last downloaded.
Attachment #8847833 - Flags: review?(nfroyd) → review+

Comment 59

3 months ago
mozreview-review
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review139218

This is kind of an rs=me, since any bugs probably mean things don't build, but a couple comments below.

::: media/libaom/generate_sources_mozbuild.sh:199
(Diff revision 6)
> +x86_platforms="--enable-postproc --as=yasm"
> +arm_platforms="--enable-runtime-cpu-detect --enable-realtime-only"
> +gen_config_files linux/x64 "--target=x86_64-linux-gcc ${all_platforms} ${x86_platforms}"
> +gen_config_files linux/ia32 "--target=x86-linux-gcc ${all_platforms} ${x86_platforms}"
> +gen_config_files mac/x64 "--target=x86_64-darwin9-gcc ${all_platforms} ${x86_platforms}"
> +gen_config_files mac/ia32 "--target=x86-darwin9-gcc ${all_platforms} ${x86_platforms}"

IIRC, we don't support 32-bit Mac anymore, so we don't need this bit?  Or does something get unhappy if it doesn't exist?

::: media/libaom/moz.build:91
(Diff revision 6)
> +        if f.endswith('.c'):
> +            if 'sse2.c' in f:
> +                SOURCES[f].flags += CONFIG['SSE2_FLAGS']

Instead of checking for `.c` suffixes first, maybe this should just be:

```
if f.endswith('sse2.c'):
    SOURCES[f].flags += ...
elif ...
```

or even:

```
flagmap = [('sse2.c', CONFIG['SSE2_FLAGS']), ...]
for suffix, flags in flagmap:
    if f.endswith(suffix):
        SOURCES[f].flags += flags
        break
```

I guess the initial filtering is more efficient, but I doubt that it matters too much.
Attachment #8857713 - Flags: review?(nfroyd) → review+

Comment 60

3 months ago
mozreview-review
Comment on attachment 8847834 [details]
Bug 1314147 - Generate build files in mach vendor aom.

https://reviewboard.mozilla.org/r/120752/#review139220
Attachment #8847834 - Flags: review?(nfroyd) → review+

Comment 61

3 months ago
mozreview-review
Comment on attachment 8859389 [details]
Bug 1314147 - Import aom library.

https://reviewboard.mozilla.org/r/131412/#review139224

rs=me
Attachment #8859389 - Flags: review?(nfroyd) → review+

Comment 62

3 months ago
mozreview-review
Comment on attachment 8858351 [details]
Bug 1314147 - Generate build description for libaom.

https://reviewboard.mozilla.org/r/130302/#review139226

It is a little unexpected (although nice) to see this split out as a separate commit, but this is not going to be the case for subsequent imports unless the importer is very careful, right?  Do we care much about that?
Attachment #8858351 - Flags: review?(nfroyd) → review+

Comment 63

3 months ago
mozreview-review
Comment on attachment 8857712 [details]
Bug 1314147 - Add --enable-av1 configure switch.

https://reviewboard.mozilla.org/r/129642/#review139228
Attachment #8857712 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 64

3 months ago
mozreview-review-reply
Comment on attachment 8858351 [details]
Bug 1314147 - Generate build description for libaom.

https://reviewboard.mozilla.org/r/130302/#review139226

I don't care much about it. I split it out to make initial review easier, but in general updates I planned to do them together. It's not too hard to split them manually, but the usual problem doing updates like this is missing some filesystem change, so I think it's better for the script the import all the changes at once.
(Assignee)

Comment 65

3 months ago
mozreview-review
Comment on attachment 8847833 [details]
Bug 1314147 - Add 'mach vendor aom' for maintaining av1 codec support.

https://reviewboard.mozilla.org/r/120750/#review139264

::: python/mozbuild/mozbuild/mach_commands.py:1841
(Diff revision 7)
> +    @CommandArgument('-r', '--revision',
> +        help='Repository tag or commit to update to.')

Yes, I wasn't sure where to put the README_MOZILLA with the source split, but I think putting it in media/libaom with the commit id and a pointer to third_party/aom will work fine.

I think having to pass `--ignore-modified` when working on the vendoring module is acceptable. One can always make a trial commit of local changes since they should be submitted in a separate patch from the vendor update anyway.

::: python/mozbuild/mozbuild/vendor_aom.py:34
(Diff revision 7)
> +        tar = tarfile.open(filename)
> +        self.log(logging.INFO, 'rm_vendor_dir', {}, 'rm -rf %s' % target)
> +        mozfile.remove(target)
> +        self.log(logging.INFO, 'unpack', {}, 'Unpacking upstream files.')
> +        tar.extractall(target)

I guess this could be something like:
```python
if any(filter(lambda name: name.startswith('/') or '..' in name, tar.getnames())):
    raise Exception('Tar archive has non-local paths')
```
The check for '..' is a little dirty, since really we should check for paths outside the toplevel.

I'm not worried because the tarfile is just a faster way of getting the contents of a particular git repo, but I don't mind adding this.

::: python/mozbuild/mozbuild/vendor_aom.py:70
(Diff revision 7)
> +        if not revision:
> +            revision = 'master'

Until upstream starts making releases, I think we'll usually want to update to master. Is this an ok default if we record the previous checkout in media/libaom/README_MOZILLA?
(Assignee)

Updated

3 months ago
Depends on: 1362107
(Assignee)

Comment 66

3 months ago
mozreview-review-reply
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review139218

> IIRC, we don't support 32-bit Mac anymore, so we don't need this bit?  Or does something get unhappy if it doesn't exist?

Good point, thanks.

> Instead of checking for `.c` suffixes first, maybe this should just be:
> 
> ```
> if f.endswith('sse2.c'):
>     SOURCES[f].flags += ...
> elif ...
> ```
> 
> or even:
> 
> ```
> flagmap = [('sse2.c', CONFIG['SSE2_FLAGS']), ...]
> for suffix, flags in flagmap:
>     if f.endswith(suffix):
>         SOURCES[f].flags += flags
>         break
> ```
> 
> I guess the initial filtering is more efficient, but I doubt that it matters too much.

Ok.
(Assignee)

Comment 67

3 months ago
mozreview-review
Comment on attachment 8859732 [details]
Bug 1314147 - Add AOMDecoder.

https://reviewboard.mozilla.org/r/131728/#review139274

::: dom/media/webm/WebMDemuxer.cpp:30
(Diff revision 3)
>  #include "mozilla/IntegerPrintfMacros.h"
>  #include "mozilla/SizePrintfMacros.h"
>  #include "mozilla/Sprintf.h"
>  
>  #include <algorithm>
> +#include <stdint.h>

In particular, we don't need two of them!

I think this is a rebase mistake, but I've opened bug 1362107 to shut my editor up about missing includes.
(Assignee)

Comment 68

3 months ago
mozreview-review
Comment on attachment 8860178 [details]
Bug 1314147 - Recognize AV1 in WebMDemuxer.

https://reviewboard.mozilla.org/r/132202/#review139294

::: dom/media/webm/WebMDemuxer.cpp:336
(Diff revision 2)
>            break;
>          case NESTEGG_CODEC_VP9:
>            mInfo.mVideo.mMimeType = "video/webm; codecs=vp9";
>            break;
> +        case NESTEGG_CODEC_AV1:
> +          mInfo.mVideo.mMimeType = "video/webm; codecs=av1";

Well, I'm unclear whether we want to signal av1-from-webm here, or just raw av1.

RFC 7741 registered video/VP8. That's what webrtc uses. There's a wg draft for video/VP9; probably waiting on the bitstream format RFC, also in draft, before publication as an RFC. I don't think the x-vnd.on2 types are more official.

AV1 doesn't have a mimetype yet. I expect the IETF netvc working group will produce a registration draft eventually, since the charter says HTML and WebRTC are intended uses. I expect it will follow what vp8 and vp9 did.

In any case this is no worse than what we have currently, so if you want to improve it, lets to that in a separate bug.

Comment 69

3 months ago
mozreview-review-reply
Comment on attachment 8860178 [details]
Bug 1314147 - Recognize AV1 in WebMDemuxer.

https://reviewboard.mozilla.org/r/132202/#review139294

> Well, I'm unclear whether we want to signal av1-from-webm here, or just raw av1.
> 
> RFC 7741 registered video/VP8. That's what webrtc uses. There's a wg draft for video/VP9; probably waiting on the bitstream format RFC, also in draft, before publication as an RFC. I don't think the x-vnd.on2 types are more official.
> 
> AV1 doesn't have a mimetype yet. I expect the IETF netvc working group will produce a registration draft eventually, since the charter says HTML and WebRTC are intended uses. I expect it will follow what vp8 and vp9 did.
> 
> In any case this is no worse than what we have currently, so if you want to improve it, lets to that in a separate bug.

the mimetype codec reported by the demuxer are to be the raw codec mimetype.

It just happened that from the start the WebM demuxer used a complex container one... But that's ugly.

That forces us to convert them back and forth in some places like here:
https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/android/AndroidDecoderModule.cpp#40

So I think it would be nice, as an extra task linked to this change, to use the proper codec mimetype, and not the container ones.

in the mean time, for av1 we could use video/av1 or video/video1...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8847834 - Attachment is obsolete: true
Attachment #8847834 - Flags: review?(kinetik)
(Assignee)

Comment 79

3 months ago
mozreview-review
Comment on attachment 8847833 [details]
Bug 1314147 - Add 'mach vendor aom' for maintaining av1 codec support.

https://reviewboard.mozilla.org/r/120750/#review139404

Would be good if you could look at this again, Nathan.
(Assignee)

Comment 80

3 months ago
mozreview-review-reply
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review139218

> Good point, thanks.

I've removed the mac/ia32 config.

Comment 81

3 months ago
mozreview-review
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review139474
Attachment #8857713 - Flags: review?(kinetik) → review+

Comment 82

3 months ago
mozreview-review
Comment on attachment 8858351 [details]
Bug 1314147 - Generate build description for libaom.

https://reviewboard.mozilla.org/r/130302/#review139480
Attachment #8858351 - Flags: review?(kinetik) → review+

Comment 83

3 months ago
mozreview-review
Comment on attachment 8859389 [details]
Bug 1314147 - Import aom library.

https://reviewboard.mozilla.org/r/131412/#review139484
Attachment #8859389 - Flags: review?(kinetik) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #79)
> Comment on attachment 8847833 [details]
> Bug 1314147 - Add 'mach vendor aom' for maintaining av1 codec support.
> 
> https://reviewboard.mozilla.org/r/120750/#review139404
> 
> Would be good if you could look at this again, Nathan.

WFM, thank you!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8860180 - Attachment is obsolete: true
(Assignee)

Comment 93

3 months ago
mozreview-review-reply
Comment on attachment 8859732 [details]
Bug 1314147 - Add AOMDecoder.

https://reviewboard.mozilla.org/r/131728/#review139144

> maybe using config() or PodZero to ensure the structure is fully initialised?

Ok. We were initializing all the fields, but you're correct that PodZero is safer against future changes.

> with MediaResult additional information can be returned to fully describe where it failed and why... such as returning the AOM error code.

Ok.

> This is rather unnecessary as the MediaDataDecoder is already ref counted and those are automally moz_count

I've removed the counters.
(Assignee)

Updated

3 months ago
Blocks: 1362465
(Assignee)

Comment 94

3 months ago
mozreview-review-reply
Comment on attachment 8857713 [details]
Bug 1314147 - Port the libvpx mozbuild generator to aom.

https://reviewboard.mozilla.org/r/129644/#review134722

> `VPX_USE_YASM` isn't defined for `--with-system-libvpx`. Maybe replace with `HAVE_YASM` + `INTEL_ARCHITECTURE` or adjust old-configure.in while keeping in mind `--with-system-aom` maybe added in future.
> 
> ```
> /usr/bin/clang -std=gnu99 -o aom_convolve_copy_sse2.o -fPIC -Wa,--noexecstack -include objdir/mozilla-config.h -DMOZILLA_CLIENT  -Imedia/libaom/config/linux/x64/ -I. -Ithird_party/aom  -c third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm
> clang: warning: argument unused during compilation: '-std=gnu99' [-Wunused-command-line-argument]
> clang: warning: argument unused during compilation: '-include objdir/mozilla-config.h' [-Wunused-command-line-argument]
> clang: warning: argument unused during compilation: '-D MOZILLA_CLIENT' [-Wunused-command-line-argument]
> third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm:2:17: error: unexpected token in argument list
> ; Copyright (c) 2016, Alliance for Open Media. All rights reserved
>                 ^
> third_party/aom/aom_dsp/x86/aom_convolve_copy_sse2.asm:4:15: error: unexpected token in argument list
> ; This source code is subject to the terms of the BSD 2 Clause License and
>               ^
> [...]
> ```

Since --enable-av1 isn't on by default, I want to delay fixing this until after this patch set lands. I've removed the VPX_USE_YASM dependency here and filed bug 1362465 for the follow-up with the other two VPX asm config keys. Thanks again for looking after these issues.

Comment 95

3 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3398e9327710
Port the libvpx mozbuild generator to aom. r=froydnj,kinetik
https://hg.mozilla.org/integration/autoland/rev/ea693af1b5fb
Add 'mach vendor aom' for maintaining av1 codec support. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e9cfbd2b4fe2
Import aom library. r=froydnj,kinetik
https://hg.mozilla.org/integration/autoland/rev/69beada73635
Generate build description for libaom. r=froydnj,kinetik
https://hg.mozilla.org/integration/autoland/rev/c57f94d10914
Add --enable-av1 configure switch. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/95e800f66f2b
Add AOMDecoder. r=jya
https://hg.mozilla.org/integration/autoland/rev/d5f91c51a3ab
Add AOMDecoder to AgnosticDecoderModule. r=jya
https://hg.mozilla.org/integration/autoland/rev/5c767e2e3ded
Recognize AV1 in WebMDemuxer. r=jya

Comment 96

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3398e9327710
https://hg.mozilla.org/mozilla-central/rev/ea693af1b5fb
https://hg.mozilla.org/mozilla-central/rev/e9cfbd2b4fe2
https://hg.mozilla.org/mozilla-central/rev/69beada73635
https://hg.mozilla.org/mozilla-central/rev/c57f94d10914
https://hg.mozilla.org/mozilla-central/rev/95e800f66f2b
https://hg.mozilla.org/mozilla-central/rev/d5f91c51a3ab
https://hg.mozilla.org/mozilla-central/rev/5c767e2e3ded
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Ralph, should this get any manual testing? If yes, can you provide some details?
Flags: needinfo?(giles)
(Assignee)

Comment 98

3 months ago
No manual testing is necessary at the moment. The codec isn't enabled by default yet, and the format is still evolving.

Thanks for checking!
Flags: needinfo?(giles)
(Assignee)

Updated

3 months ago
Blocks: 1364237
(Assignee)

Updated

2 months ago
Blocks: 1368838
Depends on: 1371899

Comment 99

2 months ago
Just for information - av1 was enabled for Linux (and MAC) in https://bugzilla.mozilla.org/show_bug.cgi?id=1370978 but https://people-mozilla.org/~rgiles/2017/rush_hour_444.av1.webm doesnt play in Linux  Nightly 55.0a1 (2017-06-11) (64-bit)
Browser console says 
Media resource https://people-mozilla.org/~rgiles/2017/rush_hour_444.av1.webm could not be decoded.

Media resource https://people-mozilla.org/~rgiles/2017/rush_hour_444.av1.webm could not be decoded, error: Error Code: NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004)
Details: RefPtr<mozilla::MozPromise<nsTArray<RefPtr<mozilla::MediaData> >, mozilla::MediaResult, true> > mozilla::AOMDecoder::ProcessDecode(mozilla::MediaRawData*): AOM error decoding AV1 sample: Corrupt frame detected

same result with https://people-mozilla.org/~rgiles/2017/sintel_trailer_2k_480p24.av1.webm
Oops, I bumped the decoder version in Firefox without updating the test files. I've bumped https://people-mozilla.org/~rgiles/2017/rush_hour_444.av1.webm and it plays for me now. I'll upload an updated encode of the sintel_trailer when the encode job finishes.

Thanks for testing!
You need to log in before you can comment on or make changes to this bug.