[FFmpeg-devel] [PATCH] avformat/matroskaenc: flag discardable packets as such

[John Stebbins]

On 12/15/2017 10:00 AM, James Almer wrote:
> On 12/15/2017 2:56 PM, John Stebbins wrote:
>> On 12/13/2017 07:14 PM, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavformat/matroskaenc.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> This only has an effect when muxing h265 streams originating from the
>>> libx265 wrapper atm, as no other encoder or demuxer currently sets
>>> the flag.
>>>
>>> I compared the output of our muxer with the latest mkvmerge, and in
>>> the latter a few more SimpleBlocks were flagged as discardable than
>>> by our muxer after this patch.
>>> I can't say if our libx265 wrapper is not properly flagging what it
>>> should, or if mkvmerge's parser is flagging more frames than it
>>> should.
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index f22c2ab70c..915ef3c107 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -2100,7 +2100,8 @@ static void mkv_write_block(AVFormatContext *s, AVIOContext *pb,
>>>      MatroskaMuxContext *mkv = s->priv_data;
>>>      AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar;
>>>      uint8_t *data = NULL, *side_data = NULL;
>>> -    int offset = 0, size = pkt->size, side_data_size = 0;
>>> +    const int discardable = !!(pkt->flags & AV_PKT_FLAG_DISPOSABLE);
>>> +    int offset = 0, size = pkt->size, side_data_size = 0, flags = 0;
>>>      int64_t ts = mkv->tracks[pkt->stream_index].write_dts ? pkt->dts : pkt->pts;
>>>      uint64_t additional_id = 0;
>>>      int64_t discard_padding = 0;
>>> @@ -2160,12 +2161,15 @@ static void mkv_write_block(AVFormatContext *s, AVIOContext *pb,
>>>          blockid = MATROSKA_ID_BLOCK;
>>>      }
>>>  
>>> +    if (blockid == MATROSKA_ID_SIMPLEBLOCK)
>>> +        flags = (keyframe << 7) | discardable;
>>> +
>>>      put_ebml_id(pb, blockid);
>>>      put_ebml_num(pb, size + 4, 0);
>>>      // this assumes stream_index is less than 126
>>>      avio_w8(pb, 0x80 | track_number);
>>>      avio_wb16(pb, ts - mkv->cluster_pts);
>>> -    avio_w8(pb, (blockid == MATROSKA_ID_SIMPLEBLOCK && keyframe) ? (1 << 7) : 0);
>>> +    avio_w8(pb, flags);
>>>      avio_write(pb, data + offset, size);
>>>      if (data != pkt->data)
>>>          av_free(data);
>> LGTM. 
>>
>> FYI, I have a pending patch that does the same thing (in a slightly different way) as well as a patch for reading the discardable flag from mkv files.
> Ah, should have known that'd be the case seeing the flag was just
> introduced.
>
>   But I wanted to wait till my other patches related to the discardable
> flag were fully reviewed and
>> accepted before adding more to the list.  As a reminder, there is still a patch to set discardable flag in the x264 encoder, a patch to read the flag in mp4 and a patch to use the flag in ffplay that are not yet accepted, though there are no unresolved
>> comments for these patch submissions.
> Do you have any idea why the output of matroskaenc differs from that of
> mkvmerge after this patch? Does libx265 (or the avcodec wrapper) not
> flag all B frames as disposable, or is mkvmerge flagging the wrong frames?
>

I'm surprised mkvmerge would flag *any* frames as disposable without the help of container level flags to tell it which
frames are not referenced.  Not all B frames are disposable.  And it's pretty difficult to determine which frames are
not referenced without actually decoding the frames (at least that's what I thought with my limited understanding of
hevc codec).

I ran some tests and mkvmerge is definitely marking the incorrect frames as disposable.  I'm able to see this with my
patches that reads the flag in matroskadec and use the flag in ffplay.  The weird thing is it is marking disposable
frames with the same cadence as a correctly marked file, but it's one frame too early.  I.e. it is marking a B-Ref and
the subsequent B frame as disposable and *not* marking the B frame that follows these two as disposable.  So It's not
just blindly marking B frames as disposable.

-- 
John      GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171215/13be72bd/attachment.sig>