| Message ID | 20171214031413.9756-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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.
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);
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.