> On Jan 20, 2023, at 10:17 AM, Tomas Härdin <g...@haerdin.se> wrote: > > ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez: >> On 18/01/2023 14:40, Tomas Härdin wrote: >>> Creating a new subthread because I just noticed something >> >> I am a bit lost there because the line of code below is not part of >> this >> FFV1 patch. >> Additionally, none on my patches (FFV1 of MXF >> stored/sampled/displayed >> fix) modifies the discussed behavior (FFmpeg behavior would be same >> before and after this patch for MPEG-2 and AVC), so should not block >> any >> of them, and a potential fix for that should have its own patch as it >> would be a separate issue. > > True, it doesn't need to hold up this patch. But some discussion is > warranted I think. I might create a separate patchset for this. > >> >> Anyway: >> >> >>> >>>> + //Stored height >>>> mxf_write_local_tag(s, 4, 0x3202); >>>> avio_wb32(pb, stored_height>>sc->interlaced); >>>> >>> Won't this be incorrect for files whose dimensions are multiples of >>> 16 >>> but not multiples of 32? Isn't each field stored separately with >>> dimensions a multiple of 16? So while for 1080p we'll have >>> >>> StoredHeight = 1088 >>> SampledHeight = 1080 >>> >>> and 1080i: >>> >>> StoredHeight = 544 >>> SampledHeight = 540 >>> >>> Where 544 is a multiple of 16, for say 720p we have >>> >>> StoredHeight = 720 >>> SampledHeight = 720 >>> >>> but for a hypothetical 720i we'd get >>> >>> StoredHeight = 360 >>> SampledHeight = 360 >>> >>> whereas the correct values should be >>> >>> StoredHeight = 368 >>> SampledHeight = 360 >> >> AFAIK, it would depend about if the stream has a picture_structure >> frame >> (16x16 applies to the frame?) of field (16x16 applies to the field?), >> but I really don't know enough there for having a relevant opinion. >> >> I can just say that I don't change the behavior of FFmpeg in your use >> case, I found the issues when I tried a random width and height of >> FFV1 >> stream then checked with MPEG-2 Video and the sampled width was wrong >> for sure e.g. sampled width of 1920 for a stream having a width of >> 1912, >> with current FFmpeg code, and for your use case I am sure about >> nothing >> so I don't change the behavior with my patch, IMO if there is an >> issue >> with 720i MPEG-2 Video it should be in a separate topic and patch as >> it >> would modify the "stored_height = (st->codecpar->height+15)/16*16" >> current code (in my patch I just move this code), unless we are sure >> of >> what should be changed on this side and apply a fix on the way. >> Better >> to fix 1 issue and let 1 open with no change than fixing no issue >> because we wouldn't be sure for 1 of the 2. > > I suspect we are lucky because 720i doesn't really exist in the real > world, and 576i and 480i are both multiples of 32. > > IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height are > "best effort" and thus shall be encoded. Their values will depend on > FrameLayout which in turn depends on what you say - how exactly the > interlacing is done. > > TL;DR: this patchset doesn't need to be held up by this.
I'm just nudging on the consideration of merging this patch. I've been testing it over the last week with ffv1/mxf content and have found this demuxing support very helpful. Dave Rice _______________________________________________ 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".
