Hi Tristan,
thanks for taking the time reviewing.
On 12/6/24 18:39, Tristan Matthews via ffmpeg-devel wrote:
Hi (also apologies if my client mangles the inline version of the patch, it's
the first time I've tried to review an attached patch with it)...
>
+ av_log(ctx, AV_LOG_DEBUG, "RTP Packet %d in (%x), len=%d:\n",
+ seq, flags, len);
+ av_hex_dump_log(ctx, AV_LOG_TRACE, buf, FFMIN(len, 128));
I think this is probably too verbose for DEBUG level (since it's called every
packet)...maybe ifdef this bit out.
Added #ifdefs and changed level to TRACE.
+ if (is_keyframe) {
+ av_log(ctx, AV_LOG_DEBUG, "Marking FIRST packet\n");
+ aggr_hdr |= AV1F_AGGR_HDR_FIRST_PKT;
+ }
I noticed that other implementations only set this bit if we're dealing with a
keyframe that is also accompanied by a
sequence header (since an encoder may emit a keyframe without one)...the spec
is a bit vague about this but it
may be safer?
I've added scanning the frame for the sequence header to make it a bit
more safe against intra-only frames inside a GOP. Please notice that
this is not fool-proof. According to the AV1 spec, a bit-identical copy
of the I-frame sequence header MAY appear in every subsequential frame.
I am not aware of any encoder doing this, however.
After I wrote this, I revisited the condition that sets in
AV_PKT_FLAG_KEY in the AV1 parsers and librav1e and libsvtav1 seem to do
this only on KEY_FRAME, not on INTRA_ONLY or SWITCH_FRAME, which should
make it safe to NOT scan for the sequence header. So it might be up for
discussion if this is really needed or just causes unnecessary overhead.
Do the other implementations you mentioned have that kind of information
by parsing OBUs or from the AV1 encoder?
I'm putting it in a #if section for now...
+ av_log(ctx, AV_LOG_TRACE, "AV1 Frame %d in (%x), size=%d:\n",
+ rtp_ctx->seq, rtp_ctx->flags, frame_size);
+ av_hex_dump_log(ctx, AV_LOG_TRACE, frame_buf, FFMIN(frame_size, 128));
This is also too frequent/verbose I think.
Put into #ifdef.
+ if ((obu_type == AV1_OBU_TEMPORAL_DELIMITER) ||
+ (obu_type == AV1_OBU_TILE_LIST)) {
+ // ignore and remove according to spec
+ obu_ptr += obu_size;
+ continue;
+ }
You also want to ignore AV1_OBU_PADDING here I believe.
The AV1 RTP spec draft does not explicitly mention OBU_PADDING, however,
it makes sense to remove it. I've added it (with a comment).
+ av_log(ctx, AV_LOG_TRACE, "Sending FRAG packet %ld/%d, %d
OBUs\n",
+ pkt_ptr - rtp_ctx->buf, rtp_ctx->max_payload_size,
num_obus);
+ av_hex_dump_log(ctx, AV_LOG_TRACE, rtp_ctx->buf, FFMIN(pkt_ptr -
rtp_ctx->buf, 128));
Again this may be too frequent/verbose.
Done.
I also found and fixed a bug in the decoder regarding fragmented OBUs
with "unlucky" growths regarding the number of LEB bytes, when the
fragmented OBU wasn't the last one in the temporal unit.
--
Best regards, Chris
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
