| Message ID | 20200220214353.240335-1-sebh@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/ac3dec: detect fairplay encryption | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Sebastian Hubbard (2020-02-20 22:43:53) > Added id3v2_buf and id3v2_buf_size fields to AVProbeData. When an id3v2 > tag is found at the start of probed content, id3v2_buf is filled with > a pointer to the start of the tag bytes, and id3v2_buf_size with the > number of bytes in the tag. > > ac3_eac3_probe checks the tag when present for a private frame with an > owner identifier of com.apple.streaming.audioDescription, which > signals that the file is encrypted with an Apple DRM scheme. The CRC > checksums for such files are for the content post-decryption, and with > this change they are skipped when Apple DRM is detected. This enables > ffprobe to detect the stream type, even though it will not be playable. > > Signed-off-by: Sebastian Hubbard <sebh@google.com> > --- > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 9b9b634ec3..afd1b67b5c 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -448,6 +448,8 @@ typedef struct AVProbeData { > unsigned char *buf; /**< Buffer must have AVPROBE_PADDING_SIZE of extra allocated bytes filled with zero. */ > int buf_size; /**< Size of buf except extra allocated bytes */ > const char *mime_type; /**< mime_type, when known. */ > + unsigned char *id3v2_buf; /**< Bytes of id3v2 tag if one was found at start of file. */ > + int id3v2_buf_size; /**< Size of id3v2_buf */ You can't just add fields here, it's an ABI break.
I knew this was an ABI change and probably wasn't going to be acceptable, but I decided to share it for consideration anyway. There's special code in av_probe_input_format3 to skip over id3 tags for the purposes of format detection, but Apple uses id3 tags to signal information for HLS audio only tracks. Maybe this change shouldn't be made, but if it could ever be helpful for another format prober to know about leading id3 tags this is the only way I could imagine it.
Am Mi., 4. März 2020 um 17:53 Uhr schrieb Sebastian Hubbard <sebh-at-google.com@ffmpeg.org>: > > I knew this was an ABI change and probably wasn't going to be > acceptable, but I decided to share it for consideration anyway. > There's special code in av_probe_input_format3 to skip over id3 tags > for the purposes of format detection, but Apple uses id3 tags to > signal information for HLS audio only tracks. Maybe this change > shouldn't be made, but if it could ever be helpful for another format > prober to know about leading id3 tags this is the only way I could > imagine it. If the additional fields are necessary (I don't know), they could be added to the end of the struct where they do not break abi. Carl Eugen
On Wed, Mar 4, 2020 at 5:57 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Mi., 4. März 2020 um 17:53 Uhr schrieb Sebastian Hubbard > <sebh-at-google.com@ffmpeg.org>: > > > > I knew this was an ABI change and probably wasn't going to be > > acceptable, but I decided to share it for consideration anyway. > > There's special code in av_probe_input_format3 to skip over id3 tags > > for the purposes of format detection, but Apple uses id3 tags to > > signal information for HLS audio only tracks. Maybe this change > > shouldn't be made, but if it could ever be helpful for another format > > prober to know about leading id3 tags this is the only way I could > > imagine it. > > If the additional fields are necessary (I don't know), they could be > added to the end of the struct where they do not break abi. > Unfortunately, thats not true for every struct. In this case, for example, they are being added at the end, but a caller could still provide a smaller one and we would try to read/write beyond the provided struct. - Hendrik
On 3/4/2020 1:56 PM, Carl Eugen Hoyos wrote: > Am Mi., 4. März 2020 um 17:53 Uhr schrieb Sebastian Hubbard > <sebh-at-google.com@ffmpeg.org>: >> >> I knew this was an ABI change and probably wasn't going to be >> acceptable, but I decided to share it for consideration anyway. >> There's special code in av_probe_input_format3 to skip over id3 tags >> for the purposes of format detection, but Apple uses id3 tags to >> signal information for HLS audio only tracks. Maybe this change >> shouldn't be made, but if it could ever be helpful for another format >> prober to know about leading id3 tags this is the only way I could >> imagine it. > > If the additional fields are necessary (I don't know), they could be > added to the end of the struct where they do not break abi. > > Carl Eugen sizeof(AVProbeData) is part of the ABI because its doxy doesn't strictly state otherwise. This means we can't add new fields to it, even at the end, until a major bump. Else we'll be breaking ABI.
Am Mi., 4. März 2020 um 18:57 Uhr schrieb James Almer <jamrial@gmail.com>: > > On 3/4/2020 1:56 PM, Carl Eugen Hoyos wrote: > > Am Mi., 4. März 2020 um 17:53 Uhr schrieb Sebastian Hubbard > > <sebh-at-google.com@ffmpeg.org>: > >> > >> I knew this was an ABI change and probably wasn't going to be > >> acceptable, but I decided to share it for consideration anyway. > >> There's special code in av_probe_input_format3 to skip over id3 tags > >> for the purposes of format detection, but Apple uses id3 tags to > >> signal information for HLS audio only tracks. Maybe this change > >> shouldn't be made, but if it could ever be helpful for another format > >> prober to know about leading id3 tags this is the only way I could > >> imagine it. > > > > If the additional fields are necessary (I don't know), they could be > > added to the end of the struct where they do not break abi. > > > > Carl Eugen > > sizeof(AVProbeData) is part of the ABI because its doxy doesn't strictly > state otherwise. This means we can't add new fields to it, even at the > end, until a major bump. Else we'll be breaking ABI. Thank you both for the explanation! Carl Eugen
On Wed, Mar 4, 2020 at 10:47 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Mi., 4. März 2020 um 18:57 Uhr schrieb James Almer <jamrial@gmail.com>: > > > > On 3/4/2020 1:56 PM, Carl Eugen Hoyos wrote: > > > Am Mi., 4. März 2020 um 17:53 Uhr schrieb Sebastian Hubbard > > > <sebh-at-google.com@ffmpeg.org>: > > >> > > >> I knew this was an ABI change and probably wasn't going to be > > >> acceptable, but I decided to share it for consideration anyway. > > >> There's special code in av_probe_input_format3 to skip over id3 tags > > >> for the purposes of format detection, but Apple uses id3 tags to > > >> signal information for HLS audio only tracks. Maybe this change > > >> shouldn't be made, but if it could ever be helpful for another format > > >> prober to know about leading id3 tags this is the only way I could > > >> imagine it. > > > > > > If the additional fields are necessary (I don't know), they could be > > > added to the end of the struct where they do not break abi. > > > > > > Carl Eugen > > > > sizeof(AVProbeData) is part of the ABI because its doxy doesn't strictly > > state otherwise. This means we can't add new fields to it, even at the > > end, until a major bump. Else we'll be breaking ABI. > > Thank you both for the explanation! > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". Hi everyone. What is the recommended path forward here? To get past the ABI break I think this patch needs: * libavformat minor version number bump (and reset of micro version) * soname version bump Is there any other changes needed?
diff --git a/libavformat/ac3dec.c b/libavformat/ac3dec.c index 1f87939388..c29c6fb27b 100644 --- a/libavformat/ac3dec.c +++ b/libavformat/ac3dec.c @@ -24,17 +24,35 @@ #include "libavcodec/ac3_parser.h" #include "avformat.h" #include "rawdec.h" +#include "id3v2.h" +#include "avio_internal.h" static int ac3_eac3_probe(const AVProbeData *p, enum AVCodecID expected_codec_id) { int max_frames, first_frames = 0, frames; const uint8_t *buf, *buf2, *end; enum AVCodecID codec_id = AV_CODEC_ID_AC3; + AVIOContext ioctx; + AVDictionary *metadata = NULL; + ID3v2ExtraMeta *id3v2_extra_meta = NULL; + int skip_crc = 0; max_frames = 0; buf = p->buf; end = buf + p->buf_size; + if (p->id3v2_buf && p->id3v2_buf_size > 0) { + ffio_init_context(&ioctx, p->id3v2_buf, p->id3v2_buf_size, 0, NULL, NULL, NULL, NULL); + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); + if (!ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) { + if (av_dict_get(metadata, "id3v2_priv.com.apple.streaming.audioDescription", NULL, 0)) { + skip_crc = 1; + } + } + ff_id3v2_free_extra_meta(&id3v2_extra_meta); + av_dict_free(&metadata); + } + for(; buf < end; buf++) { if(buf > p->buf && !(buf[0] == 0x0B && buf[1] == 0x77) && !(buf[0] == 0x77 && buf[1] == 0x0B) ) @@ -72,10 +90,10 @@ static int ac3_eac3_probe(const AVProbeData *p, enum AVCodecID expected_codec_id buf3[i ] = buf2[i+1]; buf3[i+1] = buf2[i ]; } - if (av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf3 + 2, frame_size - 2)) + if (!skip_crc && av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf3 + 2, frame_size - 2)) break; } else { - if (av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf2 + 2, frame_size - 2)) + if (!skip_crc && av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf2 + 2, frame_size - 2)) break; } if (bitstream_id > 10) diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 9b9b634ec3..afd1b67b5c 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -448,6 +448,8 @@ typedef struct AVProbeData { unsigned char *buf; /**< Buffer must have AVPROBE_PADDING_SIZE of extra allocated bytes filled with zero. */ int buf_size; /**< Size of buf except extra allocated bytes */ const char *mime_type; /**< mime_type, when known. */ + unsigned char *id3v2_buf; /**< Bytes of id3v2 tag if one was found at start of file. */ + int id3v2_buf_size; /**< Size of id3v2_buf */ } AVProbeData; #define AVPROBE_SCORE_RETRY (AVPROBE_SCORE_MAX/4) diff --git a/libavformat/format.c b/libavformat/format.c index c47490c8eb..cd510976a6 100644 --- a/libavformat/format.c +++ b/libavformat/format.c @@ -149,8 +149,10 @@ ff_const59 AVInputFormat *av_probe_input_format3(ff_const59 AVProbeData *pd, int if (lpd.buf_size > id3len + 16) { if (lpd.buf_size < 2LL*id3len + 16) nodat = ID3_ALMOST_GREATER_PROBE; - lpd.buf += id3len; - lpd.buf_size -= id3len; + lpd.id3v2_buf = lpd.buf; + lpd.id3v2_buf_size = id3len; + lpd.buf += id3len; + lpd.buf_size -= id3len; } else if (id3len >= PROBE_BUF_MAX) { nodat = ID3_GREATER_MAX_PROBE; } else
Added id3v2_buf and id3v2_buf_size fields to AVProbeData. When an id3v2 tag is found at the start of probed content, id3v2_buf is filled with a pointer to the start of the tag bytes, and id3v2_buf_size with the number of bytes in the tag. ac3_eac3_probe checks the tag when present for a private frame with an owner identifier of com.apple.streaming.audioDescription, which signals that the file is encrypted with an Apple DRM scheme. The CRC checksums for such files are for the content post-decryption, and with this change they are skipped when Apple DRM is detected. This enables ffprobe to detect the stream type, even though it will not be playable. Signed-off-by: Sebastian Hubbard <sebh@google.com> --- libavformat/ac3dec.c | 22 ++++++++++++++++++++-- libavformat/avformat.h | 2 ++ libavformat/format.c | 6 ++++-- 3 files changed, 26 insertions(+), 4 deletions(-)