1188150 - Create ADTSContainerParser

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

Add an ADTSContainerParser subclass of ContainerParser for checking header/media presence in MSE buffers.

This doesn't need to handle timestamps. The ADTS format doesn't have internal timestamps, and the spec places responsibility for generating them on the demuxer.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Part 1 - Move ContainerParser dtor to implementation

Turns out you were right. This needs to be in the implementation file anyway. Setting =default in the header still requires access to the member variable class definitions whenever ContainerParser is deallocated.

I didn't notice this in bug 1186257 because I was focussed on fixing the non-unified build there.

Comment 2

[Ralph Giles (:rillian)]

Attachment: Part 2 - ADTSContainerParser

Comment 3

[Ralph Giles (:rillian)]

Attachment: Part 3 - Instantiate ADTSContainerParser for audio/aac.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Part 4 - ADTSContainerParser gtest

Started a new test directory. dom/media/gtest is getting crowded.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639970 [details] [diff] [review]
Part 1 - Move ContainerParser dtor to implementation

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

I find {} as body definition much clearer.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639972 [details] [diff] [review]
Part 2 - ADTSContainerParser

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

::: dom/media/mediasource/ContainerParser.cpp
@@ +407,5 @@
> +    bool have_crc;
> +  } Header;
> +
> +  /// Helper to parse the ADTS header,
> +  /// returning data we care about.

Note about false value return means

@@ +476,5 @@
> +    if (!ParseHeader(aData, header)) {
> +      return false;
> +    }
> +    if (aData->Length() < header.frame_length) {
> +      MSE_DEBUGV(ADTSContainerParser, "Not enough data for for %llu byte frame"

for for

@@ +485,5 @@
> +    }
> +
> +    // Looks like we have a full packet.
> +    return true;
> +  }

not sure if it matters with ADTS as the segment are so small, but IsMediaSegmentPresent() aim isn't to tell if we have a complete media segment or not.

But if the data starts with one.

Same with IsInitSegmentPresent()

They are also expected to skip data to be ignored.


MSE will later make later the distinction of having a full segment (media or header) by calling the relevant:
  // Returns the byte range of the first complete init segment, or an empty
  // range if not complete.
  MediaByteRange InitSegmentRange();
  // Returns the byte range of the first complete media segment header,
  // or an empty range if not complete.
  MediaByteRange MediaHeaderRange();
  // Returns the byte range of the first complete media segment or an empty
  // range if not complete.
  MediaByteRange MediaSegmentRange();

So we could have IsMediaSegmentPresent() returning true ; while MediaSegmentRange() returns an empty range, which indicates a partial media segment.
In a similar fashion, MediaHeaderRange() will return an empty range if the media segment header is incomplete.

https://w3c.github.io/media-source/mpeg-audio-byte-stream-format.html
provides useful information, in particular, annex 5:
The MPEG audio byte stream is a combination of one or more MPEG Audio Frames and zero or more Metadata Frames.

    Every MPEG Audio Frame is a random access point.
    Every MPEG Audio Frame header is an initialization segment.
    The coded audio in each MPEG Audio Frame is a media segment.

@@ +492,5 @@
> +                                  int64_t& aStart, int64_t& aEnd) override
> +  {
> +    // We don't do anything here. ADTS has no embedded timestamps
> +    // so we rely on the demuxer to generate them from accumulated
> +    // frame lengths.

oh, but you do need to do things.

You need to parse the data and update the value of:
  bool mHasInitData;
  MediaByteRange mCompleteInitSegmentRange;
  MediaByteRange mCompleteMediaHeaderRange;
  MediaByteRange mCompleteMediaSegmentRange;

so that the functions:
  bool HasCompleteInitData();
  // Returns the byte range of the first complete init segment, or an empty
  // range if not complete.
  MediaByteRange InitSegmentRange();
  // Returns the byte range of the first complete media segment header,
  // or an empty range if not complete.
  MediaByteRange MediaHeaderRange();
  // Returns the byte range of the first complete media segment or an empty
  // range if not complete.
  MediaByteRange MediaSegmentRange();

return proper information.

Comment 7

[Ralph Giles (:rillian)]

Attachment: Part 2 - ADTSContainerParser v2

Address review comments. Thanks!

Comment 8

[Ralph Giles (:rillian)]

Attachment: Part 4 - ADTSContainerParser gtest v2

I think you're right that we won't really know if this is working until I've done the demuxer, but I tried to incorporate my newer understanding of how this interface is supposed to work in more gtests. Feel free to let me know where I'm still confused!

Still, I'd like to land this if/when it looks reasonable and build on it later.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640831 [details] [diff] [review]
Part 2 - ADTSContainerParser v2

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

::: dom/media/mediasource/ContainerParser.cpp
@@ +515,5 @@
> +    // start of a media segment, whether or not it's complete. So
> +    // return true if we have any data beyond the header.
> +    if (aData->Length() <= header.header_length) {
> +      return false;
> +    }

what if we only have a media segment ? (like the header was provided earlier).

Or are we to assume that a media segment is always, within the same buffer preceded by a header. Should had a comment about that.

@@ +546,5 @@
> +      return false;
> +    }
> +    mCompleteMediaSegmentRange = MediaByteRange(header.header_length,
> +                                                header.frame_length);
> +    mCompleteMediaHeaderRange = mCompleteMediaSegmentRange;

so a media header is the same as a media segment ?

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640837 [details] [diff] [review]
Part 4 - ADTSContainerParser gtest v2

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

::: dom/media/mediasource/gtest/TestContainerParser.cpp
@@ +87,5 @@
> +
> +  EXPECT_EQ(parser->InitSegmentRange(), MediaByteRange(0, header->Length()));
> +  // Media segment range should be empty here.
> +  EXPECT_EQ(parser->MediaHeaderRange(), MediaByteRange());
> +  EXPECT_EQ(parser->MediaSegmentRange(), MediaByteRange());

Any tests with media segment ?

seems to be all about testing the header here.

Comment 11

[Ralph Giles (:rillian)]

(In reply to Jean-Yves Avenard [:jya] from comment #9)

> Or are we to assume that a media segment is always, within the same buffer
> preceded by a header. Should had a comment about that.

That's what I'm assuming. I actually think it would make more sense in ADTS to say the 'initialization segment' is empty, and each media segment starts with a 'media segment header' which is the ADTS header, followed by the aac data which is the 'decodable frame data' completing the media segment. But that's not how the byte stream spec is written. 

I've added a comment documenting the assumption. If we need to handle the init segment separately we can re-parse the data out of mInitData instead.

> so a media header is the same as a media segment ?

That's what I did. None of the specs describe what a media header is, and the way the ADTS Media Stream Format is written, it can't be the ADTS header, which is the most obvious thing in context. So if we assume we always have the ADTS header together wit the AAC data in the same segment, we can choose to make the media header the same as either the media segment for the initialization segment (or some subset of the AAC data?). Treating it the same as the media segment works if we switch to separate initialization segments, so that seemed to make more sense. Will this cause problems with the MSE implementation?

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=29008 to get clarification on the media segment header definition.

> Any tests with media segment?

No. So far I'm just testing the header.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ee5d8841d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8bc93d561b
https://hg.mozilla.org/integration/mozilla-inbound/rev/032bfd7a5e75
https://hg.mozilla.org/integration/mozilla-inbound/rev/0edf0a39a23c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7ee5d8841d0
https://hg.mozilla.org/mozilla-central/rev/6b8bc93d561b
https://hg.mozilla.org/mozilla-central/rev/032bfd7a5e75
https://hg.mozilla.org/mozilla-central/rev/0edf0a39a23c