1169212 - Create ADTSDemuxer, a MediaDataDemuxer

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

As per http://w3c.github.io/media-source/byte-stream-format-registry.html

A full MSE implementation must support audio/aac and audio/mpeg.

A raw AAC stream is embedded in an Audio Data Transport Stream (ADTS) stream.

It's a very simplistic container.

We should have a ADTSDemuxer that implements the MediaDataDemuxer API ; as we already have an AAC decoder, thanks to the new media architecture added in bug 1148292 which gives us decoder re-usability, this is all we need to support raw AAC embedded in an ADTS stream.

This will also resolve bug 809186.

Comment 1

[Jean-Yves Avenard [:jya]]

Assigning it to :rillian as per IRC discussion. Feel free to re-assign it to me if you have more important issues to deal with.

Some working notes:
relevant link:
http://w3c.github.io/media-source/mpeg-audio-byte-stream-format.html
http://en.wikipedia.org/wiki/MPEG-4_Part_3
ADTS is defined in ISO/IEC 14496-3:2009

We only care about the ADTS part, which is a table in that ISO.

http://wiki.multimedia.cx/index.php?title=ADTS contains it all I believe.
A header is 7 or 9 bytes long, it defines the frequency, the number of channels, the length of the data and the number of frames (usually 1 AAC frame per ADTS frame).
then you have the raw AAC data.

ADTS doesn't contain timestamp, those are to be simulated starting from 0.
When used within MSE, those timestamps do not matter as they will be re-calculated on the fly (as per MSE spec) and this is handled by the MSE source buffer.

Comment 2

[Jean-Yves Avenard [:jya]]

For support within MSE, we will need a ADTS ContainerParser.
The two key methods now required are:
IsInitSegmentPresent() returns a bool if there's the start of an init segment
IsMediaSegmentPresent() returns a bool if there's the start of a media segment header
ParseStartAndEndTimestamps() Is called with data and returns a boolean indicating of a full media segment header was found. The other arguments are obsolete and won't be used.

a call to ParseStartAndEndTimestamps() is to update the value of:
ContainerParser::mHasInitData : bool set to true if an init segment has been found (partial or complete).
ContainerParser::mInitData : Is a fallible array containing the init segment (and only that)
ContainerParser::mCompleteByteRange : Contains a MediaByteRange that is null if we have an incomplete media segment, and set to the byte range of the first full media segment found.

Comment 3

[Ralph Giles (:rillian)]

Thanks for the update, Jean-Yves. I will finally be able to work on this next week.

Comment 4

[Ralph Giles (:rillian)]

Some push-back on this.

> A full MSE implementation must support audio/aac and audio/mpeg.

I didn't think there were any manditory formats. YouTube is certainly using aac in mp4.

Comment 5

[Ralph Giles (:rillian)]

I've not been getting to this, and at today's triage concensus is that we shouldn't support this.

Comment 6

[Jean-Yves Avenard [:jya]]

well, I'll do it then.

Playing raw aac is important imho.

While I agree AAC with MSE is likely useless, playing raw AAC is important enough

Comment 7

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

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> well, I'll do it then.
> 
> Playing raw aac is important imho.
> 
> While I agree AAC with MSE is likely useless, playing raw AAC is important
> enough

This isn't a resourcing issue. Supporting raw AAC could encourage people to use AAC instead of other codecs.

Comment 8

[Jean-Yves Avenard [:jya]]

Would that be something you'd be interesting to write?

ADTS is a very simple container. Test stream would be found in 1190341 or like http://streaming.novaentertainment.com.au/nova100

Comment 9

[Guang-De Lin]

It seems like a good topic for me.

I could start to work on this.
Please feel free to take over it if this bug gets higher priority.

Comment 10

[u480271]

Attachment: ADTS test

This is a test built upon information reported on #auckland

Comment 11

[u480271]

Attachment: Part 1: Implemented ADTS Decoder & Demuxer.

Comment 12

[u480271]

Attachment: Part 2: Advertise support for ADTS via DecoderTraits.

Comment 13

[u480271]

Attachment: Part 3: Add files to build and fix unified build.

Comment 14

[u480271]

Attachment: Part 4: Remove extended codec specific data.

Comment 15

[u480271]

With these 4 patches, I was able to listen to http://player.gridstream.org/ on windows build.

Comment 16

[u480271]

Attachment: Part 1: Implemented ADTS Decoder & Demuxer.

Implemented based upon MP3 Demuxer & Decoder.

Comment 17

[u480271]

Refreshed patches. Works on Windows/Mac with player in Bug 1190341.

Try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c9c0bfab11

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8693382 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

Review of attachment 8693382 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM at first glance.

that FFmpeg doesn't work indicate a problem however

::: dom/media/ADTSDecoder.h
@@ +17,5 @@
> +  explicit ADTSDecoder(MediaDecoderOwner* aOwner) : MediaDecoder(aOwner) {}
> +  MediaDecoder* Clone(MediaDecoderOwner* aOwner) override;
> +  MediaDecoderStateMachine* CreateStateMachine() override;
> +
> +  // Returns true if the MP3 backend is pref'ed on, and we're running on a

ADTS?

::: dom/media/ADTSDemuxer.cpp
@@ +80,5 @@
> +
> +bool
> +ADTSDemuxer::IsSeekable() const
> +{
> +  return true;

I would assume that for more live stream this would probably be false

@@ +97,5 @@
> +{
> +  // TODO:
> +  NS_WARNING("Unimplemented function NotifyDataRemoved");
> +  ADTSLOGV("NotifyDataRemoved()");
> +}

You don't need those two without MSE, the default implementations are sufficient.

@@ +117,5 @@
> +
> +bool
> +ADTSTrackDemuxer::Init()
> +{
> +  Reset();

call to reset appears redundant

@@ +125,5 @@
> +
> +  ADTSLOG("Init StreamLength()=%" PRId64 " first-frame-found=%d",
> +          StreamLength(), !!frame);
> +
> +  if (!frame)

if (!frame) {
  return false;
}

@@ +131,5 @@
> +
> +  // Rewind back to the stream begin to avoid dropping the first frame.
> +  FastSeek(media::TimeUnit());
> +
> +  if (!mInfo)

if (!mInfo) {

@@ +141,5 @@
> +  mInfo->mDuration = Duration().ToMicroseconds();
> +
> +  // AAC Specific information
> +  mInfo->mMimeType = "audio/mp4a-latm";
> +  mInfo->mProfile = 0;

you'll need to also set ExtendedProfile.
But why do you set this to 0?

This appears to be defined in byte E
: "E 	2 	profile, the MPEG-4 Audio Object Type minus 1 " from your header

@@ +616,5 @@
> +bool
> +FrameParser::FrameHeader::IsValid() const
> +{
> +  return mFrameLength > 0;
> +}

I would put all of those into the class definition.. as they really do nothing but access private members

@@ +621,5 @@
> +
> +void
> +FrameParser::FrameHeader::Reset()
> +{
> +  memset(this, 0, sizeof(FrameHeader));

Use PodZero macro

@@ +649,5 @@
> +    96000, 88200, 64000, 48000,
> +    44100, 32000, 24000, 22050,
> +    16000, 12000, 11025,  8000,
> +    7350, 0
> +  };

you only have 14 defined, why set the array size at 16?
would need to set the remaining two values at 0 and check that mSamplingIndex is valid

@@ +653,5 @@
> +  };
> +  mSampleRate = SAMPLE_RATES[mSamplingIndex];
> +
> +  static const int32_t CHANNELS[8] = {
> +    0, 1, 2, 3, 4, 5, 6, 8

that should be 7, not 8 as 8 appears reserved.

0 also means that the value is defined in-band

@@ +656,5 @@
> +  static const int32_t CHANNELS[8] = {
> +    0, 1, 2, 3, 4, 5, 6, 8
> +  };
> +
> +  mChannels = CHANNELS[mChannelConfig];

that table appears useless anyway, as CHANNELS[x] = x

::: dom/media/ADTSDemuxer.h
@@ +13,5 @@
> +#include "MediaResource.h"
> +#include "mp4_demuxer/ByteReader.h"
> +
> +namespace mozilla {
> +namespace adts {

do not use a dedicated namespace.

The reason for a dedicated namespace with MP3 was unique due to duplicated code.

@@ +71,5 @@
> +// P        2             Number of AAC frames(RDBs) in ADTS frame minus 1, for
> +//                        maximum compatibility always use 1 AAC frame per ADTS
> +//                        frame
> +// Q        16            CRC if protection absent is 0
> +class FrameParser {

FrameParser being a private declaration in the body of the code would be more elegant.

@@ +101,5 @@
> +    // Number of audio channels.
> +    int32_t Channels() const;
> +
> +    // Samples per frames
> +    int32_t SamplesPerFrame() const;

None of those methods can return a negative numbers and an undefined data is 0.
so those should be unsigned int

@@ +138,5 @@
> +
> +    // Resets the frame header and data.
> +    void Reset();
> +
> +    // Returns whether the valid

missing something here.

@@ +237,5 @@
> +
> +  // Skips the next frame given the provided byte range.
> +  bool SkipNextFrame(const FrameParser::Frame& aFrame);
> +
> +  // Returns the next MPEG frame, if available.

that would be an ADTS frame, not MPEG.

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8692331 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

Review of attachment 8692331 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/DecoderTraits.cpp
@@ +679,5 @@
>    if (IsMP3SupportedType(aType)) {
>      decoderReader = new MediaFormatReader(aDecoder, new mp3::MP3Demuxer(aDecoder->GetResource()));
>    } else
> +  if (IsAACSupportedType(aType)) {
> +    decoderReader = new MediaFormatReader(aDecoder, new adts::ADTSDemuxer(aDecoder->GetResource()));

no adts namespace

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8692332 [details] [diff] [review]
Part 3: Add files to build and fix unified build.

Review of attachment 8692332 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include <MediaStreamGraphImpl.h>
> +#include "mozilla/dom/AudioContext.h"

hmmm what?

shouldn't this be in another bug?

::: dom/media/moz.build
@@ +192,5 @@
>  ]
>  
>  UNIFIED_SOURCES += [
> +    'ADTSDecoder.cpp',
> +    'ADTSDemuxer.cpp',

this needs to be in bug 1

Comment 21

[Jean-Yves Avenard [:jya]]

Attachment: Part 4. FFmpeg fix.

I believe that this is still the wrong approach, mCodecSpecificConfig should be properly set

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8692331 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

Review of attachment 8692331 [details] [diff] [review]:
-----------------------------------------------------------------

Actually it's missing the handling in DecoderTraits::CanHandleCodecsType and DecoderTraits::CanHandleMediaType

Comment 23

[u480271]

Attachment: Part 4: Fix creation of AudioSpecificConfig.

Comment 24

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> Comment on attachment 8692332 [details] [diff] [review]
> Part 3: Add files to build and fix unified build.
> 
> Review of attachment 8692332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/GraphDriver.cpp
> @@ +3,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include <MediaStreamGraphImpl.h>
> > +#include "mozilla/dom/AudioContext.h"
> 
> hmmm what?

Exactly. I think this happened: Adding ADTSDemuxer/ADTSDecoder changed the distribution of file into unified source. ie. GraphDriver.cpp moved from unified_src_1.cpp to unified_src_2.cpp. The header is no longer already included by previous file and *boom* compile error.

All 3 parts are going to have to be folded into one to compile.  I split apart to make the review into logical parts.

Comment 25

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> @@ +656,5 @@
> > +  static const int32_t CHANNELS[8] = {
> > +    0, 1, 2, 3, 4, 5, 6, 8
> > +  };
> > +
> > +  mChannels = CHANNELS[mChannelConfig];
> 
> that table appears useless anyway, as CHANNELS[x] = x

Incorrect.

From spec, `1.6.3.5 channelConfiguration`
0

Comment 26

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> @@ +656,5 @@
> > +  static const int32_t CHANNELS[8] = {
> > +    0, 1, 2, 3, 4, 5, 6, 8
> > +  };
> > +
> > +  mChannels = CHANNELS[mChannelConfig];
> 
> that table appears useless anyway, as CHANNELS[x] = x

Incorrect.

From spec, `1.6.3.5 channelConfiguration`
Value - # of Channels
  0   -   -            defined in-band
  1   -   1
  2   -   2
  3   -   3
  4   -   4
  5   -   5
  6   -   5+1 (6)
  7   -   7+1 (8)
 8-15 -   -            reserved

I can replace the table with arithmetic, if you'd prefer.

If we're going to need the channel configuration and not let the decoder parse the in-band information itself, then well need to parse the PCE block in the raw AAC data.

Comment 27

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> Comment on attachment 8693382 [details] [diff] [review]
> Part 1: Implemented ADTS Decoder & Demuxer.
> 
> Review of attachment 8693382 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +649,5 @@
> > +    96000, 88200, 64000, 48000,
> > +    44100, 32000, 24000, 22050,
> > +    16000, 12000, 11025,  8000,
> > +    7350, 0
> > +  };
> 
> you only have 14 defined, why set the array size at 16?
> would need to set the remaining two values at 0 and check that
> mSamplingIndex is valid

mSamplingIndex is only 4 bits. Setting the size of the array to 16 allows me to index the table without checking the index. The C standard states that any initialisers not specified are set to 0. For example, see https://www.eskimo.com/~scs/cclass/notes/sx4aa.html

Comment 28

[u480271]

Attachment: Part 1: Implemented ADTS Decoder & Demuxer.

Implemented based upon MP3 Demuxer & Decoder.

Comment 29

[u480271]

Attachment: Part 2: Advertise support for ADTS via DecoderTraits.

for mimetype audio/aac & audio/aacp

Comment 30

[u480271]

Attachment: Part 2: Advertise support for ADTS via DecoderTraits.

for mimetype audio/aacp

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8695117 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

Review of attachment 8695117 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/media/DecoderTraits.cpp
@@ +372,5 @@
> +static bool
> +IsAACSupportedType(const nsACString& aType,
> +                   const nsAString& aCodecs = EmptyString())
> +{
> +#ifdef MOZ_OMX_DECODER

Only reason MP3 had a limit was to force the use of the OMX Reader on B2G as it uses low-power mode.

remove that entire conditional

Comment 32

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #31)
> Comment on attachment 8695117 [details] [diff] [review]
> Part 2: Advertise support for ADTS via DecoderTraits.
> 
> Review of attachment 8695117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> ::: dom/media/DecoderTraits.cpp
> @@ +372,5 @@
> > +static bool
> > +IsAACSupportedType(const nsACString& aType,
> > +                   const nsAString& aCodecs = EmptyString())
> > +{
> > +#ifdef MOZ_OMX_DECODER
> 
> Only reason MP3 had a limit was to force the use of the OMX Reader on B2G as
> it uses low-power mode.
> 
> remove that entire conditional

Cool. I just copied what ever MP3Decoder/MP3Demuxer did.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/864aa7951565
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3bd2dae19a

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/864aa7951565
https://hg.mozilla.org/mozilla-central/rev/ba3bd2dae19a

Comment 36

[Shian-Yow Wu [:swu]]

Hi jya, thanks for this patch.  As some apps for FxOS Smart TV need "audio/aacp" support, could you help to uplift this patch to Gecko 44 which is used by FxOS 2.5?

Comment 37

[Jean-Yves Avenard [:jya]]

The patch stack is small, feel free to apply for uplift...

Note that this is an untested feature. We have no mochitest of any kind and testing has been done by Dan and myself and that's it.

Comment 38

[Michael Ellis]

Can someone confirm this was uplifted to Gecko 44?

Comment 39

[Shian-Yow Wu [:swu]]

Comment on attachment 8695031 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: audio/aacp mime type is not supported
Testing completed: local test
Risk to taking this patch (and alternatives if risky): small according to comment 37
String or UUID changes made by this patch: n/a

Comment 40

[Josh Cheng [:josh]]

Comment on attachment 8695031 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

Approved for TV 2.5

Comment 41

[Josh Cheng [:josh]]

Comment on attachment 8695117 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

Approved for TV 2.5

Comment 42

[Josh Cheng [:josh]]

Hi tomcat,
Could you help to uplift this bug to b2g44? Thanks!

Comment 43

[Carsten Book [:Tomcat]]

part 2 has problems with uplifting to b2g44 

grafting 318731:ba3bd2dae19a "Bug 1169212 - Part 2: Advertise support for ADTS via DecoderTraits. r=jya"
merging dom/media/DecoderTraits.cpp
merging dom/media/test/test_can_play_type_mpeg.html
warning: conflicts while merging dom/media/DecoderTraits.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 44

[Jean-Yves Avenard [:jya]]

Forgot to ni dan on this one

Comment 45

[u480271]

Attachment: Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)

Rebased patch part 1 onto head of B2G44_V2_5 branch.

Comment 46

[u480271]

Attachment: Part-2 Advertise-support-for-ADTS-via-DecoderTraits (B2G44)

Rebased patch part 2 onto head of B2G44_V2_5 branch.

Comment 47

[Carsten Book [:Tomcat]]

thanks !

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d8ad34bbcc3f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fbcb121fdd50

Comment 48

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #47)
> thanks !
> 
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d8ad34bbcc3f
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fbcb121fdd50

had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=23271&repo=mozilla-b2g44_v2_5

Comment 49

[Carsten Book [:Tomcat]]

dan, can you help us here ?

Comment 50

[u480271]

Attachment: Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)

Sorry for the delay. I've uploaded a refresh part 1 patch (B2G44)

The issue turned out to be caused by a change in the unified build layout of cpp files. (Different to the order on central)

A forward decl fixed that.

I tested locally by compiling for b2g emulator-jb.

Comment 51

[Carsten Book [:Tomcat]]

also on 2.5 now 

remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f43cda156390
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4f61016ffaa1

Comment 52

[Shian-Yow Wu [:swu]]

Hi Dan, there is a typo which can cause compiling error, could you fix it?

https://dxr.mozilla.org/mozilla-central/source/dom/media/ADTSDemuxer.cpp#23

Comment 53

[Kershaw Chang [:kershaw]]

Comment on attachment 8714158 [details] [diff] [review]
Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)

Review of attachment 8714158 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/ADTSDemuxer.cpp
@@ +19,5 @@
> +#define ADTSLOGV(msg, ...) \
> +  MOZ_LOG(gADTSDemuxerLog, LogLevel::Verbose, ("ADTSDemuxer " msg, ##__VA_ARGS__))
> +#else
> +#define ADTSLOG(msg, ...) do {} while (false)
> +#define ADTSLOG(msg, ...) do {} while (false)

I think this is a typo. We should also define ADTSLOGV.

Comment 54

[u480271]

(In reply to Kershaw Chang [:kershaw] from comment #53)
> Comment on attachment 8714158 [details] [diff] [review]
> Part-1 Implemented-ADTS-Deco
> > +#define ADTSLOG(msg, ...) do {} while (false)
> > +#define ADTSLOG(msg, ...) do {} while (false)
> 
> I think this is a typo. We should also define ADTSLOGV.

Yeah that should be ADTSLOG & ADTSLOGV.

Comment 55

[u480271]

Attachment: Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)

Comment 56

[u480271]

I've refreshed Part 1 for B2G. Can you test again?

Comment 57

[u480271]

Attachment: Part 3: Fix ADTSLOGV macro definitions.

Comment 58

[Kershaw Chang [:kershaw]]

(In reply to Dan Glastonbury :kamidphish from comment #56)
> I've refreshed Part 1 for B2G. Can you test again?

I've compiled again with this patch and the error is fixed.
Thanks for your help.

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/329b4c3f90df

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/329b4c3f90df