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

Submitted by James Almer on Dec. 14, 2017, 3:14 a.m.

Details

Message ID 20171214031413.9756-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Dec. 14, 2017, 3:14 a.m.
Signed-off-by: James Almer <jamrial@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.

Comments

John Stebbins Dec. 15, 2017, 5:56 p.m.
On 12/13/2017 07:14 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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.  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.
James Almer Dec. 15, 2017, 6 p.m.
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@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?
John Stebbins Dec. 15, 2017, 7:37 p.m.
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@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.

Patch hide | download patch | download mbox

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);