Seemingly missing changes in a Git commit

Advertisements

So I am not quite sure if I am experiencing a problem with Git, or more likely that I am not quite understanding how Git commits work.
The problem I am trying to resolve is to check whether a version of FFmpeg has resolved an issue or not. The commit below should resolve a particular issue

https://github.com/FFmpeg/FFmpeg/commit/4a71da0f3ab7f5542decd11c81994f849d5b2c78

This is all well and good until you go to one of the tags containing the commit and see that it is only partially(?) applied it

https://github.com/FFmpeg/FFmpeg/blob/n0.9/libavcodec/cavsdec.c

Namely the if (level_code >= 0) part on line 133 is missing! The others updates are still present and trackable via ‘blame’. However ‘blame’ does not seem to have any clue that the missing change ever occurred.

Another fact which I find weird is that you can go to the 0.6.7 tag, which cherry picked the very same commit, and see it applied all of the changes this time.

https://github.com/FFmpeg/FFmpeg/blob/n0.6.7/libavcodec/cavsdec.c

https://github.com/FFmpeg/FFmpeg/commit/7a6bba627d643ba9e9cc083f21475a0035b0f06f

This does not seem to be a presentational Github bug, I tried fetching the actual release from the ffmpeg site and confirmed that the code is as is presented

https://ffmpeg.org/releases/

If someone knows what could have happened here my curiosity would very much appreciate it.

>Solution :

I think it may have been dropped in the next merge into the mainline.

If you take a look at changes to libavcodec/cavsdec.c between the commit you identified (4a71da0f3a) and the tag missing that check (n0.9), we have the following:

$ git log --oneline --graph 4a71da0f3a^..n0.9 libavcodec/cavsdec.c
*   dd8ffc1925 Merge remote-tracking branch 'qatar/master'
|\  
| * 9138a130cd lavc: use avpriv_ prefix for ff_frame_rate_tab.
| * 773375c3d0 lavc: rename ff_find_start_code to avpriv_mpv_find_start_code
* | d912e449b6 Merge remote-tracking branch 'qatar/master'
|\| 
| * 4a71da0f3a cavs: fix some crashes with invalid bitstreams
* 961a1a81d8 cavsdec: check run value validity
* 9f06c1c61e cavsdec: avoid possible crash with crafted input
* 6481a36010 cavs: fix oCERT #2011-002 FFmpeg/libavcodec insufficient boundary check
* faba79e080 Merge remote-tracking branch 'qatar/master'
* e10979ff56 Merge remote-tracking branch 'qatar/master'
* ce5e49b0c2 replace deprecated FF_*_TYPE symbols with AV_PICTURE_TYPE_*
* e7e2df27f8 Add ff_ prefix to data symbols of encoders, decoders, hwaccel, parsers, bsf.

I’d suggest the place to look is the merge commits (d912e449b6 and dd8ffc1925). Checking the changes of the first merge commit, (left and right) we have:

$ git diff -U0 d912e449b6^1 d912e449b6 libavcodec/cavsdec.c
diff --git a/libavcodec/cavsdec.c b/libavcodec/cavsdec.c
index fcfe06e2ce..fedee8bf72 100644
--- a/libavcodec/cavsdec.c
+++ b/libavcodec/cavsdec.c
@@ -195 +195,2 @@ static int decode_mb_i(AVSContext *h, int cbp_code) {
-    int block, pred_mode_uv;
+    unsigned pred_mode_uv;
+    int block;
@@ -450,0 +452,2 @@ static inline int check_for_slice(AVSContext *h) {
+        if (h->stc >= h->mb_height)
+            return 0;
@@ -665 +668 @@ static int cavs_decode_frame(AVCodecContext * avctx,void *data, int *data_size,
-        if(stc & 0xFFFFFE00)
+        if((stc & 0xFFFFFE00) || buf_ptr == buf_end)

and:

$ git diff -U0 d912e449b6^2 d912e449b6 libavcodec/cavsdec.c
diff --git a/libavcodec/cavsdec.c b/libavcodec/cavsdec.c
index 88034068ff..fedee8bf72 100644
--- a/libavcodec/cavsdec.c
+++ b/libavcodec/cavsdec.c
@@ -5 +5 @@
- * This file is part of Libav.
+ * This file is part of FFmpeg.
@@ -7 +7 @@
- * Libav is free software; you can redistribute it and/or
+ * FFmpeg is free software; you can redistribute it and/or
@@ -12 +12 @@
- * Libav is distributed in the hope that it will be useful,
+ * FFmpeg is distributed in the hope that it will be useful,
@@ -18 +18 @@
- * License along with Libav; if not, write to the Free Software
+ * License along with FFmpeg; if not, write to the Free Software
@@ -118 +118,2 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
-    int i, level_code, esc_code, level, run, mask;
+    int i, esc_code, level, mask;
+    unsigned int level_code, run;
@@ -126,0 +128,2 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
+            if(run > 64)
+                return -1;
@@ -133 +136 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
-        } else if (level_code >= 0) {
+        } else {
@@ -139,2 +141,0 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
-        } else {
-            break;
@@ -168 +169 @@ static inline int decode_residual_inter(AVSContext *h) {
-    if(cbp > 63){
+    if(cbp > 63U){
@@ -228 +229 @@ static int decode_mb_i(AVSContext *h, int cbp_code) {
-    if(cbp_code > 63){
+    if(cbp_code > 63U){

You can see that part of that second diff removes that check:

@@ -133 +136 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
-        } else if (level_code >= 0) {
+        } else {

which seems to be the culprit.

I think the reason might be hinted at in another part of that commit:

@@ -5 +5 @@
- * This file is part of Libav.
+ * This file is part of FFmpeg.

as this appears to have been part of an ongoing effort to merge parts of Libav, a fork of FFmpeg that is now abandoned, so perhaps there were merge issues, though redoing the merge myself:

$ git checkout d912e449b6^1
$ git merge --no-edit --no-ff d912e449b6^2

applies without any conflicts, but definitely doesn’t contain the change:

$ git diff -U0 d912e449b6 HEAD libavcodec/cavsdec.c
diff --git a/libavcodec/cavsdec.c b/libavcodec/cavsdec.c
index fedee8bf72..1b8fedf2b1 100644
--- a/libavcodec/cavsdec.c
+++ b/libavcodec/cavsdec.c
@@ -136 +136 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
-        } else {
+        } else if (level_code >= 0) {
@@ -141,0 +142,2 @@ static int decode_residual_block(AVSContext *h, GetBitContext *gb,
+        } else {
+            break;

It is possible that git has become a lot better at handling merges like this in the 10 years since that commit was pushed; perhaps back then it required a manual merge of some conflicts?

Leave a ReplyCancel reply