| Message ID | CAO7y9i-uL_iW10OfTszXy+xuiCmwt1vt84neQCnigLEtRY85pw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 8/20/2018 3:35 PM, Jacob Trimble wrote: > I am not entirely sure what this flag is supposed to be, since there > is no documentation where it is defined. But this was suggested by > James Almer as a fix for my encrypted Opus problems and several other > codec parsers do the same thing. It's a flag that lets the parser know what kind of frames it's getting. libavformat will set it if the source is a demuxer that guarantees the propagation of complete frames, like mp4, Matroska and Ogg. Applied, thanks.
On Wed, Aug 22, 2018 at 12:55 AM James Almer <jamrial@gmail.com> wrote: > > On 8/20/2018 3:35 PM, Jacob Trimble wrote: > > I am not entirely sure what this flag is supposed to be, since there > > is no documentation where it is defined. But this was suggested by > > James Almer as a fix for my encrypted Opus problems and several other > > codec parsers do the same thing. > > It's a flag that lets the parser know what kind of frames it's getting. > libavformat will set it if the source is a demuxer that guarantees the > propagation of complete frames, like mp4, Matroska and Ogg. > The flag should in a perfect world just be an optimization however, and not be required for things to actually work. Otherwise that would indicate a bug in the parser, or maybe a short-coming in the bitstream format to not present frames properly? - Hendrik
On 8/21/2018 7:47 PM, James Almer wrote: > On 8/20/2018 3:35 PM, Jacob Trimble wrote: >> I am not entirely sure what this flag is supposed to be, since there >> is no documentation where it is defined. But this was suggested by >> James Almer as a fix for my encrypted Opus problems and several other >> codec parsers do the same thing. > > It's a flag that lets the parser know what kind of frames it's getting. > libavformat will set it if the source is a demuxer that guarantees the > propagation of complete frames, like mp4, Matroska and Ogg. > > Applied, thanks. Had to revert this as it broke packet timestamp and duration generation for all Opus streams in Matroska and probably other containers. You can reproduce it with the mka test vectors in the FATE sample suite by doing a codec copy to the framecrc pseudo muxer. Decoding was apparently unaffected, so FATE didn't detect any regression. I don't know how to work around the issues you had with encrypted files if not with this. Maybe the Opus packet parsing could be done by the Matroska demuxer, much like the Ogg demuxer does (The only container apparently not negatively affected by the commit i just reverted).
On Thu, Aug 23, 2018 at 10:43 PM James Almer <jamrial@gmail.com> wrote: > > On 8/21/2018 7:47 PM, James Almer wrote: > > On 8/20/2018 3:35 PM, Jacob Trimble wrote: > >> I am not entirely sure what this flag is supposed to be, since there > >> is no documentation where it is defined. But this was suggested by > >> James Almer as a fix for my encrypted Opus problems and several other > >> codec parsers do the same thing. > > > > It's a flag that lets the parser know what kind of frames it's getting. > > libavformat will set it if the source is a demuxer that guarantees the > > propagation of complete frames, like mp4, Matroska and Ogg. > > > > Applied, thanks. > > Had to revert this as it broke packet timestamp and duration generation > for all Opus streams in Matroska and probably other containers. > You can reproduce it with the mka test vectors in the FATE sample suite > by doing a codec copy to the framecrc pseudo muxer. Decoding was > apparently unaffected, so FATE didn't detect any regression. > > I don't know how to work around the issues you had with encrypted files > if not with this. Maybe the Opus packet parsing could be done by the > Matroska demuxer, much like the Ogg demuxer does (The only container > apparently not negatively affected by the commit i just reverted). You could probably fix that by refactoring opus_find_frame_end to split the frame boundary finding and the actual packet header parsing (which sets the duration info). But it might be more interesting why it fails with encrypted streams in the first place. Does it encrypt the entire frame, including frame headers? Because if that would be the case, that change would not help anylonger. Maybe if a demuxer can know that, it should just set a flag to disable parsing entirely then. - Hendrik
From 87dfe4d3d21a824c0fbe71dad2ebc8672b3fd2b4 Mon Sep 17 00:00:00 2001 From: Jacob Trimble <modmaker@google.com> Date: Mon, 20 Aug 2018 11:25:27 -0700 Subject: [PATCH] avcodec/opus_parser: Handle complete frames flag. Signed-off-by: Jacob Trimble <modmaker@google.com> --- libavcodec/opus_parser.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/libavcodec/opus_parser.c b/libavcodec/opus_parser.c index 28b0933900..a145fe7793 100644 --- a/libavcodec/opus_parser.c +++ b/libavcodec/opus_parser.c @@ -170,19 +170,24 @@ static int opus_parse(AVCodecParserContext *ctx, AVCodecContext *avctx, ParseContext *pc = &s->pc; int next, header_len; - next = opus_find_frame_end(ctx, avctx, buf, buf_size, &header_len); - - if (s->ts_framing && next != AVERROR_INVALIDDATA && - ff_combine_frame(pc, next, &buf, &buf_size) < 0) { - *poutbuf = NULL; - *poutbuf_size = 0; - return buf_size; - } + if (ctx->flags & PARSER_FLAG_COMPLETE_FRAMES) { + next = buf_size; + header_len = 0; + } else { + next = opus_find_frame_end(ctx, avctx, buf, buf_size, &header_len); + + if (s->ts_framing && next != AVERROR_INVALIDDATA && + ff_combine_frame(pc, next, &buf, &buf_size) < 0) { + *poutbuf = NULL; + *poutbuf_size = 0; + return buf_size; + } - if (next == AVERROR_INVALIDDATA){ - *poutbuf = NULL; - *poutbuf_size = 0; - return buf_size; + if (next == AVERROR_INVALIDDATA){ + *poutbuf = NULL; + *poutbuf_size = 0; + return buf_size; + } } *poutbuf = buf + header_len; -- 2.18.0.865.gffc8e1a3cd6-goog