Support 5.1 (side) AAC encoded by FFmpeg (actual channel layout is stored in PCE)

[joeyparrish]

System info

Operating System: gLinux
Shaka Packager Version: v2.3.0

Issue and steps to reproduce the problem

Packager Command:

# Extract 30 seconds of surround sound from Sintel into MPEG2-TS (attached below)
ffmpeg -i Sintel.2010.4k.mkv -f mpegts -map 0:a -c:a aac -b:a 192k -t 30 surround.ts
# Package it to output.mp4
packager input=surround.ts,stream=audio,output=output.mp4 --mpd_output output.mpd

surround.zip

What is the expected result?
I would expect to see the output file generated.

What happens instead?
Instead, I get lots of this:

[0618/145709:ERROR:adts_header.cc(76)] Not implemented reached in
  virtual bool shaka::media::mp2t::AdtsHeader::Parse(const uint8_t *, size_t)
  ADTS frames with more than one data block not supported.

Followed by this:

[0618/145709:ERROR:packager_main.cc(523)] Packaging Error: 6 (END_OF_STREAM): 

And no output.mp4 is generated.

If Instead, I convert the 5.1 surround into stereo, everything works:

# Use -ac 2 this time to convert down to stereo (attached below)
ffmpeg -i Sintel.2010.4k.mkv -f mpegts -map 0:a -c:a aac -b:a 192k -t 30 -ac 2 stereo.ts
packager input=stereo.ts,stream=audio,output=output.mp4 --mpd_output output.mpd

stereo.zip

[kqyang]

@joeyparrish Thanks for filing the issue. We do not support more than one data block in an ADTS frames right now.

It is a nice feature to support. Move to backlog for now until we have resources to work on it.

[PhilBax]

Thought I'd take a quick look at this issue...

@joeyparrish:

If I download your .ts file, I can reproduce the problem. Interestingly, if I open your file in VLC, I hear some odd bit of music that is very stuttery (about 0.25 seconds of silence, 0.25 seconds of music, repeated through all 30 seconds of the clip). VLC claims the audio is ADTS stereo... so I'm wondering if I'm missing a codec or something.

I downloaded the 720p mkv file from here: https://download.blender.org/durian/movies/ as I'm currently taking a look at this at work, and the internet here is apparently worthless (it was going to take 22+ hrs to download the 4k version). I use the same command you list:

ffmpeg -i Sintel.2010.720p.mkv -f mpegts -map 0:a -c:a aac -b:a 192k -t 30 surround_mine.ts

and I get the following towards the end of the transcode session:

Stream mapping:
  Stream #0:1 -> #0:0 (ac3 (native) -> aac (native))
Press [q] to stop, [?] for help
[mpegts @ 0x739cc0] frame size not set
Output #0, mpegts, to 'surround_mine.ts':
  Metadata:
    encoder         : Lavf57.83.100
    Chapter #0:0: start 0.000000, end 30.000000
    Metadata:
      title           : Chapter 01
    Stream #0:0(eng): Audio: aac (LC), 48000 Hz, 5.1(side), fltp, 192 kb/s
    Metadata:
      title           : AC3 5.1 @ 640 Kbps
      encoder         : Lavc57.107.100 aac
size=     815kB time=00:00:30.01 bitrate= 222.4kbits/s speed=19.9x    
video:0kB audio:704kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 15.682124%

I can play the resulting audio just fine in VLC. Running the packager with:

/packager input=../../../surround.ts,stream=audio,output=output.mp4 --mpd_output output.mpd

I get:

[0925/095454:INFO:demuxer.cc(88)] Demuxer::Run() on file '../../../surround.ts'.
[0925/095454:INFO:demuxer.cc(160)] Initialize Demuxer for file '../../../surround.ts'.
[0925/095454:INFO:single_segment_segmenter.cc(107)] Update media header (moov) and rewrite the file to 'output.mp4'.
[0925/095454:INFO:mp4_muxer.cc(171)] MP4 file 'output.mp4' finalized.
Packaging completed successfully.

The resulting mp4 plays just fine in VLC as well, and it does detect that it is AAC with channels "3F2R/LFE".

Is there something special about the 4k version of the video that I need to use it? Am I missing a codec or command-line parameter somewhere along the line? Is the "frame size is not set" message indicative of the problem, being that this problem occurs when there is more than one block per frame?

[joeyparrish]

In my experience, the blender servers are very slow. Here's an alternate link for the 4k version I'm using: https://storage.googleapis.com/shaka-streamer-assets/sample-inputs/Sintel.2010.4k.mkv

Let me see if I can reproduce your success with the 720p version, or if I can see the differences between them.

[joeyparrish]

Just checking all the details to make sure we're doing the same thing:

• The 4k, 5.1 surround copy of Sintel I have still reproduces the issue, as does my 720p copy
• I checked the MD5 sum from the blender server, and the 4k video I'm using (alternate link) is the same as the one on blender's server
• I checked the MD5 sum for the 720p copy I have, and it doesn't match the one you downloaded from blender, so I'm getting another copy to try again (ETA 30 minutes...)
• I'm running packager version v2.3.0-5bf8ad5-release from the releases page on github
• I'm running ffmpeg version 4.1.4 from the debian package 4.1.4-1~deb10u1

[PhilBax]

Oh, interesting on the MD5! I also tried with a lower quality mp4 with 5.1 from some mirror earlier yesterday and couldn't reproduce the issue with it.

I've built the packager from source. As I am able to repro the issue with your .ts file, I'm presuming it has not been fixed. But perhaps I should rolling the source back to a specific release.

I'm on CentOS, and it looks like I'm running ffmpeg 3.4.6 (CentOS is always so far behind on software versions). I can try getting a newer ffmpeg or building from source. Perhaps the older version of ffmpeg is packaging the audio differently.

I'm somewhat of a novice when it comes to the inner workings of codecs, so forgive me if I ask dumb questions (and feel free to ignore me if I'm wasting your time. I am a professional programmer with many years of experience, and I have a bit of a personal interest in this issue being fixed, so I wanted to give it a go. But I may be foolhardy in hoping that this is an issue I can jump in and learn enough about to tackle effectively). That said, I'm curious: if the audio can be packaged in within a single ADTS frame, I'm curious what's gained by changing it to use multiple frames.

In fact, https://wiki.multimedia.cx/index.php/ADTS suggests

for maximum compatibility always use 1 AAC frame per ADTS frame.

Makes me wonder if it's actually a bug in a newer version ffmpeg. Going to see if I can debug and see just how many frames are being listed in the header.

[joeyparrish]

feel free to ignore me if I'm wasting your time

Not at all! I'm so glad you started looking at this and comparing notes with me!

It could well be a bug in ffmpeg. (Or an "accidental feature" as Larry Wall once called a bug in Perl.) If we can change ffmpeg to use 1 AAC frame per ADTS frame, then I don't need this fixed in the packager at all.

I hunted quite a lot through ffmpeg's docs for a flag to change this behavior, but I never found one. Perhaps we can find the change in the code by narrowing down affected versions and diffing the source.

[PhilBax]

If it ends up being an ffmpeg bug, well... I guess I'll have to complain less about CentOS always being so far behind in software versions in the future, as that may have revealed the source of the issue! :D

Perhaps we can find the change in the code by narrowing down affected versions and diffing the source.

Hopefully so! Going to try debugging just to see what value I get for the frame count. Also, the 4k vid just downloaded, so I'll give that a whirl as well.

[PhilBax]

Looks like on the failing run, the first four ADTS packets are fine. The fifth one has a "num_blocks_minus_1" of 1.

If I comment-out the NOTIMPLEMENTED() macro and just let the function return false to skip the packet:
Frames 1-4: 0
Frames 5-7: 1
Frame 8: 2
Frame 9-11: 3
Frame 12: 1
Frame 13: 3
Frame 14: 2
Frame 15-17: 3
Frame 18: 2

There doesn't seem to be a pattern.

Actually, scratch those frame numbers. I also see multiple failure logs on startup:
Failure while parsing Mpeg2TS: (channel_configuration_ > 0) && (channel_configuration_ < kAdtsNumChannelsTableSize)

These errors are from the first 17 frames, which have a channel_configuration_ of 0. After that, the channel_configuration_ is 2, which explains why when I try to load the file in VLC, it claims the audio is in stereo.

I'm really beginning to think this might be an ffmpeg issue.

[PhilBax]

Running ffmpeg on the 4k video you linked me to produces a .ts file I can play in VLC and run through the packager just fine.

So, I'm tempted to go look at ffmpeg, or one of its codecs. Not sure how to track down which codec I'd need to look at.

Edit: according to their docs (https://www.ffmpeg.org/ffmpeg-codecs.html#aac) it's native to ffmpeg. So I'll start looking there.

I'm curious if you can play your .ts file in VLC without issue? I believe VLC uses ffmpeg for decoding, no? So if a newer version plays your file fine, then at least their encoder and decoder both handle whatever changes they've made. And I suppose that could mean there's a newer spec of the ADTS packet (or one of the outer containers; that page lists a PES container and a TS packetizer). Since shaka reads these files natively, perhaps shaka's parser might need to be updated to handle any new specs.

[PhilBax]

https://github.com/FFmpeg/FFmpeg/blob/a0ac49e38ee1d1011c394d7be67d0f08b2281526/libavcodec/adts_header.c

ff_adts_header_parse() appears to match both https://wiki.multimedia.cx/index.php/ADTS and shaka's own header parser. Trying to find where it writes all this stuff out during encoding.

[PhilBax]

Just built ffmpeg directly from source, ran the command, and got a .ts file that fails to load in VLC!

Looking at differences in the logs from the two ffmpeg versions:

• New: extra log at beginning: [aac @ 0x3ba42c0] Using a PCE to encode channel layout "5.1(side)"
• For the ts file: Old: Lavf57.83.100 encoder; New: Lavf58.33.100 encoder
• For the audio stream inside: Old: Lavc57.107.100 aac; New: Lavc58.59.100 aac
• Different sizes and bitrates
• New: extra log at end: [aac @ 0x3ba42c0] Qavg: 2509.194

[PhilBax]

I think it's the PCE log.

There's already a bug in for it: https://trac.ffmpeg.org/ticket/6974 (also https://trac.ffmpeg.org/ticket/6983) From the comments, it's been a problem since Nov 9, 2017.

A suggested workaround there is to add -af channelmap=channel_layout=5.1, which generates a surround sound file that plays in VLC and converts with shaka.

There is discussion there about the fact that mediainfo (with "-f" option) claims the satellite speaker positions are "Side" instead of "Back". I confirmed this is the case with the output file. However, it is also the case with the .ts file produced by my old copy of ffmpeg. Since we are explicitly specifying 5.1 (not 5.1(side)) to ffmpeg, this makes me think it's just an issue with mediainfo. Also confused why the audio is defaulting to 5.1(side).

Will do more research. But seems like, for the moment, you may have a workaround!

[PhilBax]

From: https://forum.videohelp.com/threads/377474-How-to-see-a-5-1-file-s-channel-layout-Channel-layout-error-with-ffmpeg

Mediainfo's "Channel positions" (not the same as " channel layout" apparently) tells me "Front L C R, Side L R LFE" no matter what file I give it, and for files whose channel layout's are certainly different.

[joeyparrish]

This is amazing. Thank you so much for helping track this down!

[PhilBax]

For sure!

Looking at the final comment on: https://trac.ffmpeg.org/ticket/6965 The fact that shaka cannot decode PCE elements may also be considered an issue here. But the fact that VLC is also choking on the content leads me to believe it really is an issue with ffmpeg. After some further testing, I may submit a bug.

[kqyang]

@PhilBax Thanks a lot for looking into this issue!

Also FFmpeg never generates ADTS frame header with number_of_raw_data_blocks_in_frame > 0. See adts_write_frame_header implementation.

So it does look like there is a bug in shaka packager: It appears that shaka packager may have mistaken PCE as an ADTS frame and trying to parse it.

I checked our logic in LooksForSyncWord, which is indeed problematic.

    if (!audio_header->IsSyncWord(cur_buf))
      continue;

    if (!audio_header->Parse(cur_buf, raw_es_size - offset))
      continue;

    // Check whether there is another frame |size| apart from the current one.
    const size_t remaining_size = static_cast<size_t>(raw_es_size - offset);
    const int kSyncWordSize = 2;
    if (remaining_size >= audio_header->GetFrameSize() + kSyncWordSize &&
        !audio_header->IsSyncWord(&cur_buf[audio_header->GetFrameSize()])) {
      continue;
    }

The code assumes that it is an ADTS frame when it sees the sync word, and then tries to parse it. There is a following check to verify that the data afterwards starts with sync word. That check should be done before parsing the frame, i.e. it should be something like this:

    if (!audio_header->IsSyncWord(cur_buf))
      continue;

    // Check whether there is another frame |size| apart from the current one.
    const size_t remaining_size = static_cast<size_t>(raw_es_size - offset);
    const int kSyncWordSize = 2;
    const int frame_size = /* Get Frame size without parsing the frame */
    if (frame_size < audio_header->GetMinFrameSize())
      continue;
    if (remaining_size < frame_size)
      return false;
    if (remaining_size >= frame_size + kSyncWordSize &&
        !audio_header->IsSyncWord(&cur_buf[frame_size])) {
      continue;
    }

    if (!audio_header->Parse(cur_buf, frame_size))
      continue;

This is actually the original logic before the refactoring in this commit: 2a2493e#diff-6ae092cf89f3ec6efdd42778e77a49b5. I think we should get that back.

@PhilBax Would you be interested in trying the above fix and see if it works?

[PhilBax]

Okay, looking into it more:

In ac3_parser.c, line 144 we have: hdr->channel_layout = avpriv_ac3_channel_layout_tab[hdr->channel_mode];

This runs the channel mode through a table to convert it to a channel layout. It converts the 5.0 channel mode to AV_CH_LAYOUT_5POINT0. The following two lines check for the lfe channel and modify it to AV_CH_LAYOUT_5POINT1. However, in ffmpeg land, this is "5.1(side)".

On the AAC side, aacenc.c line 977 checks if the incoming channel layout is supported, but the change referenced in the previous bug report made the AAC encoder only recognize AV_CH_LAYOUT_5POINT1_BACK, or your traditional "5.1".

So, I guess a question for the ffmpeg guys is: should AC3 by default be outputing "5.1(side)" or should it be updated to output "5.1"?

I think regardless, the AAC encoder is correctly handling 5.1(side) audio as something that needs PCE encoding, and shaka-packager is not properly handling that kind of audio.

So, forcing the AAC encoder to write the audio track as 5.1 instead of 5.1(side) is probably perfectly legitimate. And in addition, the shaka-packager probably should to be fixed to handle AACs with PCE encoding.

[PhilBax]

Sure thing! I'll try it out tomorrow morning and let you know what happens!

…

On Wed, Sep 25, 2019, 5:03 PM Kongqun Yang ***@***.***> wrote: @PhilBax <https://github.com/PhilBax> Thanks a lot for looking into this issue! Also FFmpeg never generates ADTS frame header with number_of_raw_data_blocks_in_frame > 0. See adts_write_frame_header <https://github.com/FFmpeg/FFmpeg/blob/a0ac49e38ee1d1011c394d7be67d0f08b2281526/libavformat/adtsenc.c#L123> implementation. So it does look like there is a bug in shaka packager: It appears that shaka packager may have mistaken PCE as an ADTS frame and trying to parse it. I checked our logic in LooksForSyncWord <https://github.com/google/shaka-packager/blob/6b4a75b6bacdac6b0b9f598c365c8f84f0a7fd70/packager/media/formats/mp2t/es_parser_audio.cc#L35>, which is indeed problematic. if (!audio_header->IsSyncWord(cur_buf)) continue; if (!audio_header->Parse(cur_buf, raw_es_size - offset)) continue; // Check whether there is another frame |size| apart from the current one. const size_t remaining_size = static_cast<size_t>(raw_es_size - offset); const int kSyncWordSize = 2; if (remaining_size >= audio_header->GetFrameSize() + kSyncWordSize && !audio_header->IsSyncWord(&cur_buf[audio_header->GetFrameSize()])) { continue; } The code assumes that it is an ADTS frame when it sees the sync word, and then tries to parse it. There is a following check to verify that the data afterwards starts with sync word. That check should be done before parsing the frame, i.e. it should be something like this: if (!audio_header->IsSyncWord(cur_buf)) continue; // Check whether there is another frame |size| apart from the current one. const size_t remaining_size = static_cast<size_t>(raw_es_size - offset); const int kSyncWordSize = 2; const int frame_size = /* Get Frame size without parsing the frame */ if (frame_size < audio_header->GetMinFrameSize()) continue; if (remaining_size < frame_size) return false; if (remaining_size >= frame_size + kSyncWordSize && !audio_header->IsSyncWord(&cur_buf[frame_size])) { continue; } if (!audio_header->Parse(cur_buf, frame_size)) continue; This is actually the original logic before the refactoring in this commit: 2a2493e#diff-6ae092cf89f3ec6efdd42778e77a49b5 <2a2493e#diff-6ae092cf89f3ec6efdd42778e77a49b5>. I think we should get that back. @PhilBax <https://github.com/PhilBax> Would you be interested in trying the above fix and see if it works? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#598>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZODHYKNU4RLOLGFEOFIGDQLPN2TANCNFSM4HZEHMVQ> .

[PhilBax]

@kqyang const int frame_size = /* Get Frame size without parsing the frame */

How should I get the frame size without parsing the frame? Especially since, as I understand it, AACs with PCE encoding have extra data in the frame, no?

[kqyang]

@PhilBax What I meant was not parsing the other irrelevant fields, e.g. something like this:

size_t GetAdtsFrameSize(const uint8_t* data, size_t num_bytes) {
if (num_bytes < 6)
return 0;
return ((static_cast(data[5]) >> 5) |
(static_cast(data[4]) << 3) |
((static_cast(data[3]) & 0x3) << 11));
}

Especially since, as I understand it, AACs with PCE encoding have extra data in the frame, no?

Yes, the frame size returned above includes PCE data.

[PhilBax]

oh! Sorry, I see that in the refactor commit you pointed me toward. Will hook it up!

[PhilBax]

Ok, so I did the following:

• In audio_header.h, added: virtual size_t GetFrameSizeWithoutParsing(const uint8_t* data, size_t num_bytes) const {return 0;}
• In adts_header.h, added an override of that fn.
• In adts_header.cc, added the implementation, copied from the old GetAdtsFrameSize().
• Copied the code you wanted me to try, and set frame size to:
  const size_t frame_size = audio_header->GetFrameSizeWithoutParsing(cur_buf, audio_header->GetMinFrameSize()); /* Get Frame size without parsing the frame */

It handles my normal ts files fine, but when I run it on the surround.ts that joey submitted higher up on this thread (with PCE encoding), I get

[0926/134433:ERROR:packager_main.cc(530)] Packaging Error: 6 (END_OF_STREAM):

:(

[kqyang]

@PhilBax Thanks for trying! Can you submit a pull request for the changes you made so I can take a look?

[PhilBax]

Done!

[kqyang]

Thanks. Were you running the debug build or release build?

If you were running release build, can you try debug build? It may give more debug information.

$ ninja -C out/Debug
$ out/Debug/packager ...

[PhilBax]

I was running Release. Debug gives me a ton of these:
[0926/153437:WARNING:adts_header.cc(67)] Failure while parsing Mpeg2TS: (channel_configuration_ > 0) && (channel_configuration_ < kAdtsNumChannelsTableSize)

Presumably because, when PCE encoding is enabled, channel_configuration is meant to be 0, as the channel configuration is defined in the PCE data.

[kqyang]

Yes, I think you are right, so we actually have two different but related problems, both caused by the PCE data.

For channel_configuration_, we should allow it to be 0, as it can happen when there is PCE data.

Can you update the code to allow it to be 0? There may be a few places you need to update.

[PhilBax]

Updated the pull request with that change.

The resulting AAC in the mp4 file now matches the one in the .ts file. That is to say, mediainfo now shows the following for the AAC track in both files:

Channel layout : L R C Cb Lb Rb

For documentation purposes, mediainfo reports:

• Channel layout: L R C LFE Ls Rs - The original 4k.mkv AC3 audio track
• Channel layout: L R C Cb Lb Rb - .ts and .mp4 file with 5.1(side)
• Channel layout: C L R Ls Rs LFE - .ts and .mp4 file with 5.1

The latest VLC (3.0.8) plays the 5.1 version of both the .ts and .mp4 fine. However, the 5.1(side) version of both the .ts and .mp4 alternates between sound and silence every ~0.25 sec. It reports the audio as being in stereo.

The default video player on CentOS (Totem) plays the 5.1 version of both the .ts and .mp4 fine. However, it cannot play the 5.1(side) .ts file. Additionally, the 5.1(side) .mp4 file plays clearly, but the everything is shifted into the left speaker (I only have stereo headphones attached to this computer). It reports the audio as being 6-channel.

Amarok plays both sets of mp4 and ts files no problem. With my stereo-only headphones, if I'm being picky, it seems like the violin is slightly quieter and to-the-left in the 5.1(side) version. Probably worth testing on a 5.1 setup.

Rythmbox behaves exactly like Totem (5.1 ts/mp4 play fine, 5.1(side) mp4 plays in left speakers, 5.1(side) ts doesn't load). I figure it must be relying on the gstreamer plugins, which is what Totem uses as well.

I'm not sure exactly what all of this means.

I don't know if ffmpeg is writing out poorly-formed AAC+PCE files, if no player is reading them in properly (except, perhaps, Amarok), or if 5.1(side) is simply not a "valid" AAC channel layout.

When it comes to shaka, specifically, the fact that VLC loads but incorrectly plays both the .ts and .mp4 versions of the 5.1(side) audio make me think shaka is behaving correctly, and simply "passing on" any issues to output file. This is backed up by the fact that Amarok also treats both the .ts and .mp4 the same. However, the fact that the gstreamer codecs won't touch the 5.1(side) .ts file, but will load the .mp4 file but shift all the audio to the left makes me think maybe shaka isn't behaving 100% as-expected.

Up to y'all whether you want to consider the issue "fixed" for now.

[kqyang]

@PhilBax Excellent! Thanks for the detailed investigation and experiments.

Do you mind doing one additional check? Can you see what happens with the mp4 muxed by FFmpeg?

$ ffmpeg -i surround.ts -c:a copy -c:v copy surround.mp4

It worth checking whether it behaves the same way as the mp4 generated by shaka with your change.

[PhilBax]

Good call! Just tried, and the ffmpeg-generated mp4 behaves the same as the shaka-generated mp4.

• 5.1 ts -> mp4:
  • VLC: works
  • Totem/Rythmbox: works
  • Amarok: works
• 5.1(side) ts -> mp4
  • VLC: stutters (as with the ts)
  • Totem/Rythmbox: plays, but shifts to left speaker
  • Amarok: plays, but violin seems quieter and slightly to the left.

[kqyang]

@PhilBax Thanks for the confirmation. I think we can merge your pull request and consider the issue "fixed". Before merging your pull request, can you clean it up? I also left a few review comments in the pull request.

[kqyang]

One last issue. We do not parse PCE to get the actual number of channels, so it will show a channel count of 0 in mp4 metadata and DASH / HLS manifests.

@joeyparrish Do you see any problems with that during playback (in Shaka Player)?

[joeyparrish]

Yes, that could break channel-count-related logic in the player. Or at least cause surprising track choices that didn't honor channel-related configs.

[synologic]

I can also confirm that Shaka Player does not play my streams which contain 5.1(side) AAC as encoded by ffmpeg