Re: [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 
>>> ---
>>>  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




signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

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 
>> ---
>>  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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[John Stebbins]

On 12/13/2017 07:14 PM, James Almer wrote:
> Signed-off-by: James Almer 
> ---
>  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.

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




signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

Signed-off-by: James Almer 
---
 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);
-- 
2.15.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel