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".

Reply via email to