Create ADTSDemuxer, a MediaDataDemuxer

RESOLVED FIXED in Firefox 45

Status

()

P5
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jya, Assigned: kamidphish)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+, firefox41 affected, firefox45 fixed, b2g-v2.5 fixed, b2g-master fixed)

Details

Attachments

(6 attachments, 10 obsolete attachments)

128 bytes, text/html
Details
34.75 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
5.95 KB, patch
jya
: review+
Details | Diff | Splinter Review
3.80 KB, patch
Details | Diff | Splinter Review
34.92 KB, patch
Details | Diff | Splinter Review
965 bytes, patch
jya
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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.
Assignee: nobody → giles
(Reporter)

Comment 2

4 years ago
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.
Thanks for the update, Jean-Yves. I will finally be able to work on this next week.
Component: Audio/Video → Audio/Video: Playback
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.
Priority: -- → P5
I've not been getting to this, and at today's triage concensus is that we shouldn't support this.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 6

3 years ago
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
Assignee: giles → jyavenard
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
(Reporter)

Comment 8

3 years ago
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
Flags: needinfo?(fs.in.nccu)

Comment 9

3 years ago
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.
Flags: needinfo?(fs.in.nccu)
Created attachment 8692315 [details]
ADTS test

This is a test built upon information reported on #auckland
Created attachment 8692330 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.
Attachment #8692330 - Flags: feedback?(jyavenard)
Created attachment 8692331 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.
Created attachment 8692332 [details] [diff] [review]
Part 3: Add files to build and fix unified build.
Created attachment 8692809 [details] [diff] [review]
Part 4: Remove extended codec specific data.
With these 4 patches, I was able to listen to http://player.gridstream.org/ on windows build.
Created attachment 8693382 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

Implemented based upon MP3 Demuxer & Decoder.
Attachment #8692330 - Attachment is obsolete: true
Attachment #8692809 - Attachment is obsolete: true
Attachment #8692330 - Flags: feedback?(jyavenard)
Refreshed patches. Works on Windows/Mac with player in Bug 1190341.

Try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c9c0bfab11
Attachment #8692331 - Flags: review?(jyavenard)
Attachment #8692332 - Flags: review?(jyavenard)
Attachment #8693382 - Flags: review?(jyavenard)
(Reporter)

Comment 18

3 years ago
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.
Attachment #8693382 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 19

3 years ago
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
Attachment #8692331 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 20

3 years ago
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
Attachment #8692332 - Flags: review?(jyavenard) → review-
(Reporter)

Comment 21

3 years ago
Created attachment 8693424 [details] [diff] [review]
Part 4. FFmpeg fix.

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

Comment 22

3 years ago
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
Attachment #8692331 - Flags: review+ → review-
Created attachment 8693433 [details] [diff] [review]
Part 4: Fix creation of AudioSpecificConfig.
(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.
(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
(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.
(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
Created attachment 8695031 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

Implemented based upon MP3 Demuxer & Decoder.
Attachment #8695031 - Flags: review+
Attachment #8692332 - Attachment is obsolete: true
Attachment #8693382 - Attachment is obsolete: true
Attachment #8693424 - Attachment is obsolete: true
Attachment #8693433 - Attachment is obsolete: true
Created attachment 8695032 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

for mimetype audio/aac & audio/aacp
Attachment #8695032 - Flags: review?(jyavenard)
Attachment #8692331 - Attachment is obsolete: true
Created attachment 8695117 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

for mimetype audio/aacp
Attachment #8695117 - Flags: review?(jyavenard)
Attachment #8695032 - Attachment is obsolete: true
Attachment #8695032 - Flags: review?(jyavenard)
(Reporter)

Comment 31

3 years ago
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
(Reporter)

Updated

3 years ago
Attachment #8695117 - Flags: review?(jyavenard) → review+
(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 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/864aa7951565
https://hg.mozilla.org/mozilla-central/rev/ba3bd2dae19a
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1232552
See Also: → bug 1232552
(Reporter)

Updated

3 years ago
See Also: bug 1232552
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?
Flags: needinfo?(jyavenard)
(Reporter)

Comment 37

3 years ago
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.
Flags: needinfo?(jyavenard)

Comment 38

3 years ago
Can someone confirm this was uplifted to Gecko 44?
Flags: needinfo?(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
Attachment #8695031 - Flags: approval‑mozilla‑b2g44?

Updated

3 years ago
Attachment #8695117 - Flags: approval‑mozilla‑b2g44?

Updated

3 years ago
Flags: needinfo?(swu)
Comment on attachment 8695031 [details] [diff] [review]
Part 1: Implemented ADTS Decoder & Demuxer.

Approved for TV 2.5
Attachment #8695031 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+

Updated

3 years ago
feature-b2g: --- → 2.5+

Updated

3 years ago
status-b2g-v2.5: --- → affected
status-b2g-master: --- → fixed
Comment on attachment 8695117 [details] [diff] [review]
Part 2: Advertise support for ADTS via DecoderTraits.

Approved for TV 2.5
Attachment #8695117 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Hi tomcat,
Could you help to uplift this bug to b2g44? Thanks!
Flags: needinfo?(cbook)
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)
Flags: needinfo?(cbook) → needinfo?(jyavenard)
(Reporter)

Comment 44

3 years ago
Forgot to ni dan on this one
Flags: needinfo?(jyavenard) → needinfo?(dglastonbury)
Created attachment 8707268 [details] [diff] [review]
Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)

Rebased patch part 1 onto head of B2G44_V2_5 branch.
Created attachment 8707269 [details] [diff] [review]
Part-2 Advertise-support-for-ADTS-via-DecoderTraits (B2G44)

Rebased patch part 2 onto head of B2G44_V2_5 branch.
Flags: needinfo?(dglastonbury) → needinfo?(cbook)
(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
status-b2g-v2.5: fixed → affected
Flags: needinfo?(jyavenard)
(Reporter)

Updated

3 years ago
Flags: needinfo?(jyavenard) → needinfo?(dglastonbury)
dan, can you help us here ?
(Reporter)

Updated

3 years ago
Assignee: jyavenard → dglastonbury
Created attachment 8714158 [details] [diff] [review]
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.
Attachment #8707268 - Attachment is obsolete: true
Flags: needinfo?(dglastonbury) → needinfo?(cbook)
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
Flags: needinfo?(dglastonbury)
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.
(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.
Flags: needinfo?(dglastonbury)
Created attachment 8715727 [details] [diff] [review]
Part-1 Implemented-ADTS-Decoder-Demuxer (B2G44)
Attachment #8714158 - Attachment is obsolete: true
I've refreshed Part 1 for B2G. Can you test again?
Flags: needinfo?(swu)
Created attachment 8715737 [details] [diff] [review]
Part 3: Fix ADTSLOGV macro definitions.
Attachment #8715737 - Flags: review?(jyavenard)
(Reporter)

Updated

3 years ago
Attachment #8715737 - Flags: review?(jyavenard) → review+
(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.
Flags: needinfo?(swu)
You need to log in before you can comment on or make changes to this bug.