[FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

I just wanted to send a final ping about this patch. While most
streams will not encounter the case that it addresses, those that do
will have a pretty bad experience. The demuxer's read_packet function
currently only reads 7 bytes (ADTS_HEADER_SIZE) of input. If the first
12 bits aren't the sync word, it immediately gives up and returns
AVERROR_INVALIDDATA. This means that if it encounters an ID3 tag, it
won't re-sync unless the tag length just happens to be a multiple of 7
bytes.

I'm assuming that the demuxer should be enhanced to search input for
the next sync word. (That might be some of the additional work that
James mentioned, and is something I'd be interested in working on.) In
the mean time, I think this patch addresses a likely case of lost
synchronization, while also providing some utility to API users.

I know that people are probably busy, so I don't want to be annoying
or overly persistent. However, if anyone has a little time to review
this and provide feedback or accept the changes, I'd appreciate it.

Thanks,

-Richard


On Wed, Feb 7, 2018 at 9:40 AM, Richard Shaffer <rshaffer at tunein.com> wrote:
> I did send the sample file and a unit test for this. If there are no
> issues or other comments, would you guys be willing to merge it?
>
> -Richard
>
> On Fri, Feb 2, 2018 at 11:15 AM, Richard Shaffer <rshaffer at tunein.com> wrote:
>> On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamrial at gmail.com> wrote:
>>> On 2/1/2018 11:37 PM, rshaffer at tunein.com wrote:
>>>> From: Richard Shaffer <rshaffer at tunein.com>
>>>>
>>>> While rare, ID3 tags may be inserted between ADTS frames. This change enables
>>>> parsing them and setting the appropriate metadata updated event flag.
>>>> ---
>>>> I have encountered a streaming provider that I must support which does this.
>>>> There are indications that it isn't totally off the wall, and that it does
>>>> happen in the wild:
>>>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>>>   (See specifically sections 3 and 4.)
>>>> * https://github.com/video-dev/hls.js/issues/508
>>>>
>>>> That being said, the providers that do this are in the minority. It seems most
>>>> streaming providers will do one of the following:
>>>>  1. If using .aac segments inside of HLS, use the #EXTINF for text metadata and
>>>>     use an ID3 tag at the head of the segment for things like timing metadata.
>>>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>>>
>>>> Aside from comments on the code, I'd be interested in any opinion about whether
>>>> this is something that should or should not be supported in libavformat.
>>>>
>>>> Thanks,
>>>>
>>>> -Richard
>>>
>>> Can you provide a sample and add a fate test for this? To avoid
>>> regressions in the future. This demuxer still needs some work.
>>>
>>> Use the new probetags() function you added in your other id3v2 test if
>>> the tags between samples in the file are the only ones, or if they
>>> differ in some way from the ones at the beginning of the file, so
>>> ffprobe will report them instead.
>>> Alternatively, see fate-adts-demux in tests/fate/demux.mak
>>
>> Oh, somebody did merge that test. How cool.
>>
>> I don't think we can test this with ffprobe, though. It will just read
>> tags at the head of the file and not in the middle.
>>
>> I have at least one sample that's ok for me to share. I'll work on a
>> separate patch to add a fate test.
>>
>> -Richard
>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel