[FFmpeg-devel] [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket

[Tomas Härdin]

lör 2024-02-03 klockan 20:58 +0100 skrev Tomas Härdin:
> lör 2024-02-03 klockan 11:51 +0100 skrev Jerome Martinez:
> > On 03/02/2024 11:00, Tomas Härdin wrote:
> > > fre 2024-02-02 klockan 16:55 +0100 skrev Jerome Martinez:
> > > > Before this patch, the FFmpeg MXF parser correctly detects
> > > > content
> > > > with
> > > > 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the
> > > > FFmpeg
> > > > JPEG
> > > > 2000 decoder reads the JPEG 2000 SIZ header without
> > > > understanding
> > > > that
> > > > the indicated height is the height of 1 field only so
> > > > overwrites
> > > > the
> > > > frame size info with e.g. 720x243, and also completely discards
> > > > the
> > > > second frame, which lead to the decoding of only half of the
> > > > stored
> > > > content as "progressive" 720x243 flagged interlaced.
> > > Is the decoder really the right place to do this? Surely this
> > > happens
> > > with more codecs than just j2k. Isnt it also a parser's job to do
> > > this
> > > kind of stuff?
> > 
> > Both solutions have pro and con:
> > - Doing it in the decoder fixes the issue for all containers and
> > only
> > one codec
> > - Doing it in the demuxer fixes the issue for one container and all
> > codecs
> > And currently I know only one container and only one codec having
> > this 
> > issue.
> 
> A more proper fix might be to auto-insert a deinterleave filter in
> ffmpeg.
> 
> > My choice to implement in the decoder comes from the idea that it
> > seems 
> > more hacky to decode 2 AVPackets (crafted from a single MXF
> > packet), 
> > then do a RAM copy of the decoded (so huge) content for
> > interleaving 
> > fields into 1 frame (pressure on RAM bandwidth) in the MXF demuxer
> > + 
> > adapt frame metadata (height is overwritten by the decoder then
> > need
> > to 
> > overwrite again the value), doing it in the decoder seems less
> > intrusive.
> 
> The fastest way, in a player, is probably to do it with a shader.
> That
> should be the least amount of copies and the most cache coherent.
> 
> > If moving to the demuxer is the only acceptable solution, I will do
> > it 
> > but I would like to be sure that there is a consensus on that by
> > FFmpeg 
> > developers before doing it, in order to not have this extra work 
> > rejected due to a future disagreement about where it should go.
> > 
> > > The logic that scans the packet for two SOI markers shouldn't be
> > > necessary if the relevant information is carried over from the
> > > MXF
> > > demuxer.
> > 
> > As far as I know there is nothing in the MXF file saying where is
> > the
> > second field in the packet, at which MXF metadata do you think?
> 
> Well, FrameLayout == SEPARATE_FIELDS, EditRate = 30000/1001 and
> FieldDominance == 2. DisplayHeight is the field height as S377 says
> it
> should be for SEPARATE_FIELDS.

It should also be said that what this patch effectively does is
silently convert SEPARATE_FIELDS to MIXED_FIELDS. What if I want to
transcode J2K to lower bitrate but keep it SEPARATE_FIELDS?

/Tomas